rust-lang / cargo

The Rust package manager
https://doc.rust-lang.org/cargo
Apache License 2.0
12.25k stars 2.31k forks source link

Feature selection in workspace depends on the set of packages compiled #4463

Open matklad opened 6 years ago

matklad commented 6 years ago

Maintainers notes


Reproduction:

  1. Check out this commit: https://github.com/matklad/fall/commit/3022be495a8523ca43c3d78ab29966fbb5ee69fd

  2. Build some test with cargo test -p fall_test -p fall_test -p lang_rust -p lang_rust -p lang_json --verbose --no-run

  3. Build other tests with cargo test --all --verbose --no-run

  4. Run cargo test -p fall_test -p fall_test -p lang_rust -p lang_rust -p lang_json --verbose --no-run again and observe that memchr and some other dependencies are recompiled.

  5. Run cargo test --all --verbose --no-run and observe memchr recompiled again.

The verbose flag gives the following commands for memchr:

Running `rustc --crate-name memchr /home/matklad/trash/registry/src/github.com-1ecc6299db9ec823/memchr-1.0.1/src/lib.rs --crate-type lib --emit=dep-info,link -C debuginfo=2 --cfg 'feature="default"' --cfg 'feature="libc"' --cfg 'feature="use_std"' -C metadata=be49c4722e8b48bf -C extra-filename=-be49c4722e8b48bf --out-dir /home/matklad/trash/fall/target/debug/deps -L dependency=/home/matklad/trash/fall/target/debug/deps --extern libc=/home/matklad/trash/fall/target/debug/deps/liblibc-90ba32719d46f457.rlib --cap-lints allow -C target-cpu=native`
Running `rustc --crate-name memchr /home/matklad/trash/registry/src/github.com-1ecc6299db9ec823/memchr-1.0.1/src/lib.rs --crate-type lib --emit=dep-info,link -C debuginfo=2 --cfg 'feature="default"' --cfg 'feature="libc"' --cfg 'feature="use_std"' -C metadata=be49c4722e8b48bf -C extra-filename=-be49c4722e8b48bf --out-dir /home/matklad/trash/fall/target/debug/deps -L dependency=/home/matklad/trash/fall/target/debug/deps --extern libc=/home/matklad/trash/fall/target/debug/deps/liblibc-335251832eb2b7ec.rlib --cap-lints allow -C target-cpu=native`

Here's the single difference:

--extern libc=/home/matklad/trash/fall/target/debug/deps/liblibc-90ba32719d46f457.rlib 
--extern libc=/home/matklad/trash/fall/target/debug/deps/liblibc-335251832eb2b7ec.rlib 

Versions (whyyyyy cargo is 0.21 and rustc is 1.20??? This is soo confusing)

λ cargo --version --verbose
cargo 0.21.0 (5b4b8b2ae 2017-08-12)
release: 0.21.0
commit-hash: 5b4b8b2ae3f6a884099544ce66dbb41626110ece
commit-date: 2017-08-12

~/trash/fall master
λ rustc --version
rustc 1.20.0 (f3d6973f4 2017-08-27)
matklad commented 6 years ago

So, it has to do with features. Namely, two cargo invocations produce two different libcs:

Running `rustc --crate-name libc /home/matklad/trash/registry/src/github.com-1ecc6299db9ec823/libc-0.2.30/src/lib.rs --crate-type lib --emit=dep-info,link -C debuginfo=2 --cfg 'feature="use_std"' -C metadata=335251832eb2b7ec -C extra-filename=-335251832eb2b7ec --out-dir /home/matklad/trash/fall/target/debug/deps -L dependency=/home/matklad/trash/fall/target/debug/deps --cap-lints allow -C target-cpu=native`
Running `rustc --crate-name libc /home/matklad/trash/registry/src/github.com-1ecc6299db9ec823/libc-0.2.30/src/lib.rs --crate-type lib --emit=dep-info,link -C debuginfo=2 --cfg 'feature="default"' --cfg 'feature="use_std"' -C metadata=90ba32719d46f457 -C extra-filename=-90ba32719d46f457 --out-dir /home/matklad/trash/fall/target/debug/deps -L dependency=/home/matklad/trash/fall/target/debug/deps --cap-lints allow -C target-cpu=native`

The only difference is --cfg 'feature="default"'.

So, I get two different libcs in target:

λ ls target/debug/deps | grep liblibc
.rw-r--r-- 982k matklad  3 Sep 14:06 liblibc-90ba32719d46f457.rlib
.rw-r--r-- 982k matklad  3 Sep 14:03 liblibc-335251832eb2b7ec.rlib

But I get a single memchr:

λ ls target/debug/deps | grep libmemchr
.rw-r--r-- 186k matklad  3 Sep 14:09 libmemchr-be49c4722e8b48bf.rlib

The file name is the same for both cargo commands, but the actual contents differs.

matklad commented 6 years ago

Hm, so this looks like more serious then spurious rebuild!

Depending on what -p options you pass, you might end up with different final artifacts for the same package. This should not happen, right?

matklad commented 6 years ago

Minimized example here: https://github.com/matklad/workspace-vs-feaures

matklad commented 6 years ago

@alexcrichton continuing discussion here, instead of #4469 which is somewhat orthogonal, as you've rightly pointed out!

I don't think this'd be too hard to implement, but I'm not sure if this is what we'd want implemented per se. If one target of a workspace doesn't want a particular feature activated, wouldn't it be surprising if some other target present in a workspace far away activated the feature?

Yeah, it looks like what we ideally want here is that each final artifact gets the minimal set of features. And this should work even withing a single package: currently, activating feature in dev-dependecy will activate it for usual dependency as well. This is also something to keep in mind if we go the route of binary-only (or per-target) dependencies.

Though such fine-grained feature activation will cause more compilation work overall, so using union of featues might be a pragmatic choice, as long as we keep features additive, and it sort of makes sense, because crates in workspace share dependencies anyway. And seems better then definitely some random unrelated target activating features for you depending on the command line flags.

alexcrichton commented 6 years ago

I think one of the main problems right now is that we're doing feature resolution far too soon, during the crate graph resolution. Instead what we should be doing is assuming all features are activated until we actually start compiling crates. That way if you have multiple targets all requesting different sets of features they'll all get separately compiled copies with the correct set of features.

Does that make sense? Or perhaps solving a different problem?

matklad commented 6 years ago

Does that make sense? Or perhaps solving a different problem?

Yeah, totally, "they'll all get separately compiled copies with the correct set of features" is the perfect solution here, and it could be implemented by moving feature selection after the dependency resolution.

But I am really worried about additional work to get separately compiled copies, because it is multiplicative. Let's say you have a workspace with the following layout:

1) leaf crates A and B, which transitively depend on external crate libc with different features 2) A large number of intermediate crates, on which A and B also depend 3) An ubiquitous utils crate, that depends on libc and is a dependency of any other crate.

Because A and B require different features from libc, and because libc happens to be at the bottom of the dependency graph, that means that for cargo build --all we will compile every crate twice. Moreover, editing utils and then doing cargo build --all again recompiles everything two times.

So it's not that only libc will get duplicated, the whole graph may be duplicated in the worst case.

nipunn1313 commented 6 years ago

If we assume that features are additive (as intended), then the innermost crate could be compiled once with the union of all features.

Additive features are a bit of a subtle point though (see #3620). Recompiling is the safest way, though expensive.

alexcrichton commented 6 years ago

@matklad yeah you're definitely right that the more aggressively we cache the more we end up caching :). @nipunn1313 you're also right that it should be safe for features to be unioned, but they often come with runtime or linkage implications. For example if a workspace has a no_std project and an executable, compiling both you wouldn't want to enable the standard library in the dependencies of the no_std project by accident!

I basically see this as there's a specification of what Cargo should be doing here. We've got, for example, two crates in a workspace, each which activates various sets of features in shared dependencies. Today Cargo does the "thing that caches too much" if you compile each separately (and also suffers a bug when you switch between projects it recompiles too much). Cargo also does the "union all the features" if you build both crates simultaneously (e.g. cargo build --all). Basically Cargo's not consistent!

I'd advocate that Cargo should try to stick to the "caches too much" solution as it's following the letter of the law of what you wrote down for a workspace. It also means that crates in a workspace don't need to worry too much about interfering with other crates in a workspace. Projects that run into problems of the "too much is cached" nature I'd imagine could then do the investigation to figure out what features are turned on where, and try to get each workspace member to share more dependencies by unifying the features.

matklad commented 6 years ago

Projects that run into problems of the "too much is cached" nature I'd imagine could then do the investigation to figure out what features are turned on where, and try to get each workspace member to share more dependencies by unifying the features.

This somewhat resolves my concern about build times, but not entirely. I am worried that it might not be easy to unify features manually, if they are turned on by private transitive dependencies. It would be possible to do by adding this private transitive dependency as an explicit and unused dependency, but this looks accidental.

But now I too lean towards fine-grained features solution.

nipunn1313 commented 6 years ago

For what it's worth, we've done that exact trick with the parallel feature of the gcc crate. It does happen, but the workaround is ok.

On Wed, Sep 6, 2017 at 12:45 AM Aleksey Kladov notifications@github.com wrote:

Projects that run into problems of the "too much is cached" nature I'd imagine could then do the investigation to figure out what features are turned on where, and try to get each workspace member to share more dependencies by unifying the features.

This somewhat resolves my concern about build times, but not entirely. I am worried that it might not be easy to unify features manually, if they are turned on by private transitive dependencies. It would be possible to do by adding this private transitive dependency as an explicit and unused dependency, but this looks accidental.

But now I too lean towards fine-grained features solution.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/rust-lang/cargo/issues/4463#issuecomment-327403064, or mute the thread https://github.com/notifications/unsubscribe-auth/ABPXoxPIsKCCcH5DEgqtKzPt9ek34uLeks5sfk2EgaJpZM4PLGrK .

SimonSapin commented 6 years ago

Servo relies on the current behavior to some extent: two "top-level" crates (one executable and one C-compatible static library) depend on a shared library crate but enable different Cargo features. These features are mutually exclusive, enabling the union would not work.

Maybe the "right" thing to do here is to have separate workspaces for the different top-level things? Does it make sense for shared path dependencies to be members of two separate workspaces?

(Servo’s build system sets $CARGO_TARGET_DIR to different directories for the two top-level things so that they don’t overwrite each other. They also happen to be built with different compiler versions (some nightly v.s. some stable release).)

nipunn1313 commented 6 years ago

I would be in support of cargo build --all building the same dependency multiple times rather than resolving a feature union. This would be equivalent of running cargo build in a loop over each crate in the workspace. This would prevent multiple crates within a workspace from interfering with each other (or multiple dependencies in the dep-chain interfering). I believe it would cover Servo's case as well.

What makes this problem so insidious is that there's no way to enforce or even encourage the union property of features. If a project pulls in even one dependency that doesn't obey this property, it could potentially create an incorrect binary.

In @SimonSapin's case with Servo, I think Servo is lucky that the feature'd crate (style) is only one-level in from the top level crate. If you had a dep chain like

evenbiggerproject -> servo -> style[featA]
                  -> geckolib -> style[featB]

then I believe that compiling evenbiggerproject with cargo would select the union of features for style and use it for both geckolib and servo. This would be an incorrect binary w.r.t. the intent of the servo/geckolib Cargo.tomls

Our project at Dropbox ran into a similar issue with itertools -> libeither, where libeither was compiled with two different features. Lucky for us, libeither's features are union-safe, so the code was correct, but it did create spurious recompiles depending on which sub-crate we were compiling.

djc commented 6 years ago

I agree with @nipunn1313 -- I think cargo build --all should build all crates exactly as they would be if you had run cargo build for each crate separately. If that requires us to recompile some crates, so be it.

SimonSapin commented 6 years ago

This all sounds like agreement on what should happen. @alexcrichton, what code changes need to happen (on a high level) to get there?

djc commented 6 years ago

That's what I was discussing with @alexcrichton at the RustFest impl days, and I have a bunch of refactoring done that I'm still tweaking. Will post a PR ASAP. Do you have a particular dependency/urgency relating to Gecko or Servo on this?

SimonSapin commented 6 years ago

Nothing urgent. I thought this bug could cause spurious rebuilds after selectively building a crate with -p, but I couldn’t reproduce. Anyway, thanks for working on this!

nipunn1313 commented 6 years ago

We've had to fork some deps to unify feature selection to work around this issue. It's definitely not sustainable for us, but not urgent yet.

--Nipunn

On Sat, Oct 14, 2017 at 5:27 AM Simon Sapin notifications@github.com wrote:

Nothing urgent. I thought this bug could cause spurious rebuilds after selectively building a crate with -p, but I couldn’t reproduce. Anyway, thanks for working on this!

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/rust-lang/cargo/issues/4463#issuecomment-336631887, or mute the thread https://github.com/notifications/unsubscribe-auth/ABPXo7HL7OuZhSaMgZMtY9Y5IxnY7dHFks5ssKjIgaJpZM4PLGrK .

djc commented 6 years ago

@nipunn1313 for my understanding, can you point me at a commit or otherwise elaborate on what problems you've had due to this issue?

nipunn1313 commented 6 years ago

Here's an example of a problem we had to work around https://github.com/rust-lang/cargo/issues/3620#issuecomment-326462000

In that particular case, either and itertools were both present in our workspace. We ended up internally forking itertools to ask for a wider set of features from libeither, so there was consistency across the workspace.

alexcrichton commented 6 years ago

@SimonSapin taking on this issue will require a relatively significant refactoring of Cargo's backend. Right now feature resolution happens during crate graph resolution, but we need to defer it all the way until the very end when we're actually compiling crates.

driftluo commented 6 years ago

Today, I encountered a problem with the overall compilation of the workspace and the inconsistent compilation of each crate.

We have a public logger crate, the default is output to a file, open console features will be output to the terminal.

Some crates in the workspace have this feature turned on, while others do not need to be turned on.

But cargo bulid --all will cause the entire workspace's crates to be output to the terminal.

This behavior makes me very confused.

idubrov commented 5 years ago

Regarding:

Projects that run into problems of the "too much is cached" nature I'd imagine could then do the investigation to figure out what features are turned on where, and try to get each workspace member to share more dependencies by unifying the features.

  1. What would be the best way to figure out which features needs to be unified?

I hacked together a crude tool that compares output of multiple cargo build --build-plan -Z unstable-options --package XYZ runs to see if compiler would be invoked with a different set of features for the same version of dependency for different packages. Is there a better way to do it?

  1. Would it be feasible to ask for a feature to "lock" features on the workspace level, similar how versions are locked via Cargo.lock? Something along the lines (in the workspace Cargo.toml):
    [enable-features] # some better name
    # for all the dependencies of `syn` in the workspace matching version `0.15`,
    # forcefully enable "visit-mut" and "extra-traits" features.
    syn = { version = "0.15", features = ["visit-mut", "extra-traits"] }

    ?

This is pretty much to avoid crude solution I outlined here: https://users.rust-lang.org/t/forcing-set-of-features-for-shared-dependencies-across-targets-in-workspace/25899 (I don't really want to have "fake" dependencies or tune features across multiple Cargo.toml files).

  1. So far I found two use-cases for feature unification:

First, it improves compilation times (as explained in my post on users.rust-lang.org).

Second, I found that inconsistent features break the following use-case I have (which might be a not fully valid one?):

  1. shared crate is compiled as dylib
  2. plugin crate is compiled with shared as a dependency; plugin itself is dylib
  3. app crate is compiled as a binary with shared as a dependency; NO direct dependency on plugin

app tries to load plugin at runtime via libloading library.

So, when features are consistent on shared between two targets (plugin and app), everything works fine. I can build plugin and app independently, start app and it will load plugin. Code is shared between these two in the form of libshared.dylib dynamic library.

However, when features are not consistent (say, plugin=>shared=>another_dep enables some feature A in another_dep and app=>shared=>another_dep does not), this causes plugin to fail loading because some symbols would be missing in libshared.dylib. Also, it seems like a single version of libshared.dylib is created for both targets, so building app & plugin independently would constantly override that libshared.dylib.

It seems like dynamic libraries is still an unfinished story in Rust, so I don't know what really I could be asking here (it might be that it "works" only accidentally?).

retep998 commented 4 years ago

Because winapi is usually such a deep transitive dependency and has so many unique ways to select which features to enable, this problem is a particular nuisance for us Windows users because winapi gets rebuilt every time we switch which package we're building. This is most infuriating with cargo run because we need -p to select which package to run and it causes deep rebuilds all the time.

nipunn1313 commented 4 years ago

^ we have the same issue with winapi in particular. We worked around it by autogenerating our Cargo.toml to always specify the exact union of winapi features which we require (including all transitive dependencies) as a direct dependency of every Cargo.toml. There are a few other cases in which it's an issue.

It's a painful workaround for an even more painful issue.

joshtriplett commented 3 years ago

I find myself in need of this as well.

There was a recent discussion on Zulip between @ehuss, myself, and several others, which came to the following rough conclusion:

jethrogb commented 3 years ago

@joshtriplett it sounds like you want feature selection to be dependent on the set of packages compiled? That's the opposite of what this bug is about.

If what this bug is about has changed over the years, maybe a moderator could update the title & summary?

nipunn1313 commented 3 years ago

I'm not a mod - but I've been here following with this task for 3+ years now. Here's a writeup of my summary - from reading through this.

  • It should be possible to ask cargo to build all (or a subset of) the crates of a workspace with independent feature unification, such that each of the workspace's crates build their dependencies with only the minimal set of features required to satisfy that crate and its dependencies, not the whole workspace.
  • There should be a top-level option for a workspace's Cargo.toml to opt into that behavior, such that cargo build or cargo build -p ... will always have that behavior.

Sounds like there are two modes of operating on the table

Crate Specific Feature Selection (CSF):

Each crate in a workspace is built with its deps (incl vendored) compiled with the minimal features needed for just that crate

Pros

Workspace Aware Feature Selection (WAF)

Each crate in a workspace is built with its deps having the union of features needed for the whole workspace

Pros


Today, we're awkwardly in between, causing a lot of recompiles and some confusion: cargo build inside a crate within a workspace uses "CSF" cargo build --all or cargo build from workspace root uses "WAF"


Ideas/Proposals

A) Default CSF

B) Default WAF

C) Keep today's behavior - CSF in crates, WAF at root


My personal thoughts. I think (C) is a bad option - as it's leading to confusion.

W.r.t developer environment within workspace, WAF seems better for developer build times when switching crates often. CSF seems better when focusing on one crate.

W.r.t. output bloat CSF seems better if the crates are independent. WAF seems better if the crates are meant to be used together (eg one primary crate - w/ several subcrates within the workspace).

Hopefully this is helpful!

alexcrichton commented 3 years ago

I would agree that the workspace-aware unification is probably the right default, personally. When this is combined with -Zfeatures=all from today it may actually mean that basically no one wants crate-specific-features perhaps? We could always perform a transition like this gradually and switch the defaults during an edition or something like that.

For your option (A), though, I'm not sure if we could actually implement that or if it would have the desired effect. We need to unify shared dependencies somehow, and if we ended up doing separate feature resolution for crates that would cause just as many rebuilds as if you did cargo build in each workspace member. It would also be unfortunately pretty difficult to implement with Cargo's current architecture without some deeper refactorings I think.

mahkoh commented 3 years ago

I haven't read all comments so maybe this has already been noted: If we have dependencies a -> b -> c and a -> c. And a and b depend on feature c/x, then b will not compile if its Cargo.toml does not activate c/x. But if a activates c/x then a will compile because it implicitly activates c/x for its dependency b. This can be somewhat confusing.

sunshowers commented 3 years ago

I'm currently implementing the "workspace-aware unification" model described above using guppy, see the documentation for hakari for more details. We're about to ship it in Diem Core. Seems to work well for our use case though there are definitely issues around dependency bloat that we're still working through and iterating on. (We may have to eventually go with a more fine-grained approach where we unify features for certain subsets of the workspace differently, but we expect to do all this through the guppy toolset without involving Cargo.)

cbiffle commented 2 years ago

Just wanted to chime in and note that we're relying on per-crate feature unification in Hubris, so, I agree with @joshtriplett's comment that if the default were to change, there should be an override.

sunshowers commented 2 years ago

Quick followup as well: I've turned our automatic workspace-hack generator, hakari, into a command-line tool called cargo-hakari. It has a number of config knobs which:

I think any sort of unification strategy within cargo is likely going to need these knobs as well.

tamird commented 2 years ago

This unhygienic behavior is not specific to workspaces, is it? In the case shown in the cargo documentation, if my-package were to also have a direct dependency on winapi with features = [] it would also observe winapi having all the features specified by foo and bar. That means that a change in the features used by either foo or bar could produce build breakage in my-package.

nipunn1313 commented 2 years ago

This unhygienic behavior is not specific to workspaces, is it? In the case shown in the cargo documentation, if my-package were to also have a direct dependency on winapi with features = [] it would also observe winapi having all the features specified by foo and bar. That means that a change in the features used by either foo or bar could produce build breakage in my-package.

I think the problem you're describing is a different (related) problem - a designed behavior documented here https://doc.rust-lang.org/cargo/reference/features.html#feature-unification. Cargo expects features to be "additive".

martijnarts commented 1 year ago

Would it be an idea to put an alias into cargo build and cargo check to explicitly check everything in the workspace separately? Basically aliasing it to rerunning the same command a bunch of times?

I've now resorted to some find ... | xargs ... monstrosity in my CI to resolve this.

recatek commented 1 year ago

Cargo expects features to be "additive".

While this is the case, this isn't enforced by the language. There also doesn't seem to exist an equivalent that provides preprocessor-like functionality but isn't additive (e.g. for stripping some code out of certain builds, like client vs. server). Having the additive-only feature assumption baked into workspaces is a pain point I've been encountering a lot with cargo check and rust-analyzer.

JarvisCraft commented 1 year ago

Hi there!

Sharing my experience, this behavior seems to be a more serious problem since it affects build-requirements of separate independent crates.

This causes a problem when some of the crates require alloc and some don't: In flipperzero crate while working on flipperzero-rs/flipperzero#44 where there is an examples directory with multiple binary crates demonstrating the use of the API I tried to group the latter under the common workspace. While individual builds (cargo build -p foo) still work, grouped build (cargo build) fails due to feature unification:

Build log ```shell $ cargo build Compiling flipperzero-sys v0.7.0 (~/home/progrm_jarvis/CLionProjects~/flipperzero-rs/crates/sys) Compiling flipperzero-rt v0.7.0 (/home/progrm_jarvis/CLionProjects/flipperzero-rs/crates/rt) Compiling flipperzero v0.7.0 (/home/progrm_jarvis/CLionProjects/flipperzero-rs/crates/flipperzero) Compiling flipperzero-alloc v0.7.0 (/home/progrm_jarvis/CLionProjects/flipperzero-rs/crates/alloc) Compiling notification v0.1.0 (/home/progrm_jarvis/CLionProjects/flipperzero-rs/examples/notification) Compiling gui v0.1.0 (/home/progrm_jarvis/CLionProjects/flipperzero-rs/examples/gui) Compiling gpio v0.1.0 (/home/progrm_jarvis/CLionProjects/flipperzero-rs/examples/gpio) Compiling dialog v0.1.0 (/home/progrm_jarvis/CLionProjects/flipperzero-rs/examples/dialog) Compiling hello-rust v0.1.0 (/home/progrm_jarvis/CLionProjects/flipperzero-rs/examples/hello-rust) error: no global memory allocator found but one is required; link to std or add `#[global_allocator]` to a static item that implements the GlobalAlloc trait error: could not compile `hello-rust` due to previous error warning: build failed, waiting for other jobs to finish... error: could not compile `gpio` due to previous error error: could not compile `gui` due to previous error error: could not compile `notification` due to previous error ```

What seems to happen is feature unification causing all crates to have alloc feature enabled while normally only some of them (i.e. dialog) requires it. Since crates not enabling alloc feature don't specify the allocator, they fail to build when alloc feature becomes enabled on them.


Original thread on Zulip.

kriswuollett commented 10 months ago

I'm used to working in monorepos, but relatively new to Rust. Figuring out how to set up projects such that features are propagated properly took a bit of time. Perhaps this issue really isn't the problem, but rather there shouldn't be default-packages in a workspace Cargo.yaml, and one shouldn't be able to compile anything unless the package list is explicitly given. If not ambiguous, I think feature selection may resolve to something undesired if multiple packages are being asked to be built at the same time. To be more precise, it would have to be something more like Bazel transitions?

Instead it may be more precise to only have Cargo build one package which in turn is responsible for dependencies and propagating the features in a workspace environment.

In other words, Rust in a workspace should act more like how "solutions" work in an IDE like Visual Studio, i.e., a configuration that binds the target triple, feature set, and artifacts to build. I should be able to then cargo switch to a new solution and build something completely different. And also have this concept be used in IDEs so that all the conditional code inclusion gets rendered properly -- see the mentioned rust-lang/rust-analyzer#15545 linked above.

epage commented 10 months ago

@kriswuollett if I'm reading into your comments correctly, it sounds like its focused on some specific artifacts within the workspace and wanting to define the desired configuration for building those specific artifacts.

At this time, a lot of cargo is centered on building a single artifact set. There are complications with its current model for building multiple at once and I feel like a more holistic view of the problem is needed to figure out, even it should even belong in cargo (vs cargo being more neutral and expecting some other process to handle it). I've written some more on these thoughts at https://epage.github.io/blog/2023/08/are-we-gui-build-yet/

kriswuollett commented 10 months ago

@epage, thanks for the comments/link!

I fall into the camp of rustc/cargo should be usable by an external build orchestration tool like in the further linked article about Bazel. I wouldn't expect cargo to do everything for what a developer may need to do which realistically these days could be wanting to compile multiple languages including Rust into WASM modules which get embedded as bytes into a Rust binary for a different arch/glibc version that uses wasmtime that then gets put into a tar file as a layer on top of a distroless layer to put in a signed OCI image to be pushed to a registry in a single build command. I'm just avoiding Bazel at the moment so I concentrate on learning and coding with Rust first. :-)

As a related aside, the issue of being usable by a build orchestration tool is important, because if the architecture is never written that way, it may no longer be possible, likely ever, see https://github.com/flutter/flutter/issues/25377, of which otherwise I'm a fan of the platform.

Here in this issue I just wanted to put in my 2c about the devex in relation to an IDE, and that the multi-dimensional feeling about configuring targets and features makes me think compiling more than one package at once is nearly technically impossible to get "right".

RalfJung commented 3 months ago

This doesn't just affect workspaces, it also affects crates with dev-dependencies. (I think it's the same issue, anyway. Please let me know if I should file a new issue instead.)

I often work on a project that has

Working on this project often involves both cargo test and cargo run. However, unfortunately, cargo test && cargo run will build the binary twice! First cargo test will build it together with the tests, which means more features are enabled in some dependency, and then cargo run will build it again against a crate graph that has fewer features. This means cargo run often ends up taking a lot longer than it has to, given that a perfectly fine binary was already created by cargo test.

It would be great if there was some flag or so that made cargo run use the same crate graph as cargo test, so that binaries can be shared with cargo test. Even if that meant that it actually builds the test crates (which is entirely unnecessary of course) that would still save significant time compared to the status quo since it avoids building the binary twice.

(For some time I tried to ensure that the binary itself enables enough features in its dependencies to make the crate graphs identical, but it is quite tricky to figure out what the difference between the builds is and even if I manage to find all the right features at some point, it regularly breaks on cargo update. I eventually gave up on that strategy; I think the only way this is feasible is with a tool that can just automatically tell me about these differences, and that can run on CI to ensure no new differences creep in.)

sunshowers commented 3 months ago

@RalfJung (have you seen https://crates.io/crates/cargo-hakari that I authored? it automates all this for you, and should mostly prevent this kind of duplicate build)

RalfJung commented 3 months ago

I have not seen this before, thanks for the pointer!

I don't quite want a separate crate for this, just some extra dependencies on my bin crate, but maybe this can help.

Arnavion commented 3 months ago

First cargo test will build it together with the tests, which means more features are enabled in some dependency, and then cargo run will build it again against a crate graph that has fewer features.

To be clear, this is the desirable behavior for many people, including me. I do *not* want the cargo run-compiled binary to have unnecessary features and dependencies enabled and creating bloat. After all that's why I didn't enable those features in the first place. It may even be *incorrect* to enable those features, eg tests might require "std" to be able to unwrap() but the compiled binary must not depend on libstd.

It would be great if there was some flag or so that made cargo run use the same crate graph as cargo test

Yes, if it's opt-in, then there's no problem.

sunshowers commented 3 months ago

It is definitely desirable for many people to not do feature unification at times, either partially or fully. Hakari comes with several knobs to make that possible: https://docs.rs/cargo-hakari/latest/cargo_hakari/config/index.html#traversal-excludes

This is a complicated problem with no easy answers. Any solution in Cargo is going to need a ton of configuration knobs.

Hawk777 commented 2 months ago

I’ve got a use case that doesn’t appear to have been written up yet. I have some libraries with optional-but-default std features; with those features disabled, the libraries are no_std-capable. I then have a binary, which uses a subset of those libraries in no_std mode. Because the binary uses the libraries in no_std mode, it defines its own #[panic_handler]. I naïvely thought I could just put all the crates into a workspace and expect a plain cargo clippy at the workspace root to check all the crates using their individual default settings (i.e. mylib would be checked with std enabled, because that’s the default, but mybinary would be checked against mylib[-std], because that’s what it asked for). Unfortunately it doesn’t work that way; cargo clippy chooses only one feature set for mylib (which is, due it being a default feature, +std), and then the check of mybinary fails because there’s now a duplicate panic handler (one in mybinary and one in std).

I suppose one could say this is a case where the language kind of forces features to be non-additive. If we take “additive” to mean “works with the feature everywhere it would work without the feature”, then the std feature cannot be additive: it works in a no_std, panic_handler-defining binary without the feature but not with it.