Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Build Deps getting mixed in with dependencies #2589

Closed
tbelaire opened this issue Apr 17, 2016 · 33 comments
Closed

Build Deps getting mixed in with dependencies #2589

tbelaire opened this issue Apr 17, 2016 · 33 comments
Assignees
Labels
A-build-dependencies Area: [build-dependencies] A-dependency-resolution Area: dependency resolution and the resolver A-features Area: features — conditional compilation C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted`

Comments

@tbelaire
Copy link

I am trying to depend on the log crate for my rust.ko project, but log is already in use in bindgen, one of my build dependencies. bindgen uses log with the use_std feature, which is totally fine as that's running normally. However, when I add log to be build with default-features = false for the kernel module, which cannot use std, I get errors where cargo tries to build, as log tries to find std.

build transcript:
https://gist.github.com/tbelaire/a98527cf5d946e7c2fd4823dd16126a6

Cargo.lock:
https://gist.github.com/tbelaire/f0f89e8a21f9ce5498df07b6ec8e3a0c

@alexcrichton
Copy link
Member

Interesting! This is, unfortunately, intended behavior. The resolution phase of Cargo doesn't know about target-specific or build dependencies, it just resolves everything, so the standard feature activation applies where features are all unioned. That is, log isn't treated separately as part of a build dependency or as part of the main resolution graph.

To disable default features you'll have to ensure that all crates in the dependency graph don't use the default feature, regardless of whether they're part of a build script or not.

@tbelaire
Copy link
Author

I don't actually know how I would fix this though, as I can't turn off use_std for my transitive build dependencies, as it does use std! So that approach doesn't help at all.

Or when cross compiling, it should make absolutely no difference how the packages are compiled in build deps phase, as they are going to be running on a different machine afterwards, so it's not like you need to try and re-use artifacts between the two.

I feel like the build deps should be sealed off from the rest of your deps completely, as there are two completely different environments they are running in.

As a short term workaround, I could clone the log crate and create a log_dup crate which is exactly the same except for the name, and that would work, but it's incredibly frustrating, and doesn't scale if I run into other dependencies shared between them.

@alexcrichton
Copy link
Member

Unfortunately this just isn't how features work in Cargo. When you depend on something you can't depend on it only with one set of features as all other features get unioned. This means that if a dependency is shared then both dependants will receive the same feature set.

@tbelaire
Copy link
Author

I'm not happy, as at the end of the day, I can't use the log crate. I see a couple of places that might be at fault.

  1. I should not use log because it's also a transitive dependancy of one of my build depends. I think that's as silly as if make also depended on a incompatible version of log, and blocked my build.
  2. Log is misusing features, and this should have been resolved by having 3 crates, log-frontend, log-std, log-no-std, and everyone just depends on two. This would work I think, but might be technically impossible.
  3. Cargo is incorrect in mixing constraints and features from the build deps and the target deps. Perhaps there needs to be a flag isolate-build-deps available in Cargo.toml.

If I made an RFC for proposing isolate-build-deps, do you think it would gain any traction?

My possible workarounds consist of duplicating crates under different names, or moving the build.rs in to a separate crate, which then depends on things as normal, and using a makefile to co-ordinate builds between them. Neither of these solutions are particularly nice.

@alexcrichton
Copy link
Member

You should of course feel free too propose an RFC! I'd recommend trying to understand what's in play currently and think through how it can be changed first, however.

Right now features are resolved at resolution time because they can introduce new dependencies which need to be applied. Features are also not a property of an edge in a dependency graph but a property of a node, which is to say that there is no way for you to depend on log with only some features activated, but rather you have to mutate the log crate's features that everyone sees. Then, when we compile a crate, we correctly cross compile the crate for multiple architectures but the resolved feature set is the same.

Along those lines, and RFC could perhaps tweak aspects like:

  • Having use_std as a feature simply isn't compatible, instead crates should be used. (e.g. the libstd facade).
  • If a crate is compiled twice (e.g. once for the host and once for the target) then the feature set changes based on what other crates in the same "target graph" (e.g. those compiled for the same target) activate. That is if build scripts activate the std feature, but no target crate does, then the two compilations of log are the same. This is incompatible, however, with cargo build which does not have a target/host distinction.

etc.

@tbelaire
Copy link
Author

I think the approach I'd take with the RFC would be to push to isolate build deps in the resolution phase.

Essentially, I think it a project like

.
├── Cargo.toml
├── build.rs
└── src
    └── lib.rs

could be transformed into something looking like

.
├── build-dep
│   ├── Cargo.toml // dependencies = build-deps
│   └── src
│       └── main.rs  // renamed from build.rs
└── project
    ├── Cargo.toml
    └── src
        └── lib.rs

where we run the main binary from build-dep in the directory of project automatically just like build.rs's binary, but are otherwise isolated. I don't see any glaring semantic problems with this approach, and it would solve my problem.

Of course, it would be nice to optimize for the common case where it's actually okay to share dependencies and not rebuild everything twice.
But maybe that could use the workspace stuff? Can you only share if the versions/features happen to line up, and double compile if they are in conflict?

Because I feel like the problem here is that there really are two distinct trees of dependencies, and any sharing they do is incidental.

@tbelaire
Copy link
Author

Oh, and I feel like features are actually not the main part of this, as I also had problems when the Cargo.lock was out of sync, and each was trying to compile a different version of the log crate (though I fixed this by just deleting the lock files before really investigating, so I could be wrong). I think it's possible to recreate this problem with only conflicting version requirements, and no mention of features.

@alexcrichton
Copy link
Member

The "desugaring" you mentioned above is basically what happens today, the crux of this is indeed feature sharing. The same node in the dependency graph, log, has only one set of features activated (in this case use_std). The dependency graph does not express host/target compiles, so there's not two nodes in the graph for the log crate to have different sets of features.

@pka
Copy link

pka commented Jun 8, 2016

I think I have the same problem, trying to use different features on Linux and Windows. I tried to declare it in different ways, here's the most explicit:

[target.'cfg(unix)'.dependencies]
postgres = { version = "*", features = ["unix_socket"] }
unix_socket = "*"

[target.'cfg(windows)'.dependencies]
postgres = { version = "*", features = [] }

But cargo tries to compile unix_socket on Windows as well and fails.
Isn't there a way to avoid the unix_socket dependency on Windows?

@alexcrichton
Copy link
Member

@pka yeah unfortunately that's the same issue as this. When we build the resolution graph today it contains information about all targets, and the filtering per platform only happens at the very end when we're compiling.

cc @sfackler though, perhaps unix-socket could be a noop on windows?

@nox
Copy link
Contributor

nox commented Jun 26, 2016

It seems to me that this also is causing a blocker for Servo, because it makes https://github.com/serde-rs/serde#using-serde-with-stable-rust-and-serde_codegen not work anymore.

@alexcrichton You seem to be saying it has always been this way, but are you sure? How does anyone let users decide whether they want to go through serde_macros or just serde_codegen then?

@nox
Copy link
Contributor

nox commented Jun 26, 2016

Try this on webrender_traits:

cargo build -v --no-default-features --features serde_macros

The intended result is that serde_codegen doesn't get build with syntex_syntax because it then gets pulled in only with default features through serde_macros. Unfortunately it doesn't happen and the with-syntex feature of serde_codegen still gets pulled in.

@nox
Copy link
Contributor

nox commented Jun 26, 2016

@pka yeah unfortunately that's the same issue as this. When we build the resolution graph today it contains information about all targets, and the filtering per platform only happens at the very end when we're compiling.

This sounds like a regression because Serde seems to rely on this idiom.

@alexcrichton
Copy link
Member

@nox I was under the impression that servo would only have one dependency on serde_codegen which because features are unioned would disallow a copy with syntex and a copy with syntax. That is, I think this issue may not be applicable because nothing is trying to use serde_codegen at runtime, it's only as a build dependency, right?

@jdub
Copy link
Contributor

jdub commented Oct 9, 2016

Oof, I just hit this with log and bindgen too, with an extra surprise: cargo's single dependency graph spans cross-compile targets.

Do you foresee (a correct implementation of) separate dependency graphs causing any build problems or compatibility issues?

@cyplo
Copy link

cyplo commented Mar 4, 2017

Hello ! Any consensus/news on this one ?

@ghost
Copy link

ghost commented Mar 20, 2017

I just hit the same issue while cross-compiling for wasm32-unknown-emscripten.
(In this case it is possible to remove features in the build script to allow it to compile, but this may not always be the case.)

@fallen751
Copy link

fallen751 commented Sep 5, 2017

I wanted to second the fact that this is quite annoying to deal with when cross compiling. It makes using build scripts in a no_std environment quite cumbersome. Right now the "best" solution is to use manually specify the crate git in your [dependencies] section so you get two separate dependencies nodes. In general, it's very unintuitive that build dependencies for your host target and real dependencies for your cross-compilation target with different feature sets get resolved to the same dependency by cargo.

@carols10cents carols10cents added A-build-dependencies Area: [build-dependencies] A-dependency-resolution Area: dependency resolution and the resolver A-features Area: features — conditional compilation C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` labels Sep 25, 2017
@withoutboats
Copy link
Contributor

We talked about this in the cargo team meeting today.

What surprises me about this is that we generate a single resolution graph and then figure out what kind of dependencies they are. I would expect that we would generate a resolution graph for build dependencies totally separate from normal dependencies. Then, when generating the lock file and build plan, after figuring out features and so on, we merge the sets of dependency nodes from the two graphs, to dedup dependencies where possible (but this is an optimization).

@valff
Copy link

valff commented Feb 16, 2018

I ran into the same issue in no_std environment. In my case even specifying two versions, one from crates.io, another from github, doesn't work:

[dependencies.failure]
version = "0.1"
default-features = false

[build-dependencies.failure]
git = "https://github.com/withoutboats/failure"
branch = "version-0.1"
default-features = false
features = ["std"]

It fails with Dependency 'failure' has different source paths depending on the build target. Each dependency must have a single canonical source path irrespective of build target.

I can't use std feature in my lib, but I want to use std in my build.rs. So I guess for now there is no option to solve it, except publishing a copy of failure crate under a different name?

@emilio
Copy link

emilio commented Oct 15, 2018

@alexcrichton is there any update on this? Is there any chance you or anybody else could take it or explain a path forward to fixing this?

This is a major pain for build-time dependencies, since it prevents us from using a lot of crates, or worse yet, from updating them.

To be honest I'd rather spend a couple weeks fixing this than having to land stuff like mozilla/cbindgen#222, for example...

@emilio
Copy link

emilio commented Oct 15, 2018

(Read that as an "I'm volunteering to take a look in my free time to this as long as I can ask questions to somebody" :P)

@ehuss
Copy link
Contributor

ehuss commented Oct 16, 2018

@emilio I've been looking at this recently. The solution may take a while since it may require extensive changes. I feel like it's a high priority issue, so I'm going to continue pushing to make something happen.

@emilio
Copy link

emilio commented Oct 16, 2018

@ehuss that's amazing to know, thank you! Let me know if you have cargo-begginner-friendly-ish work you could get a hand with :)

teskje added a commit to teskje/cstr_core that referenced this issue Oct 26, 2018
This is a temporary fix for https://gitlab.com/ra_kete/kmod-rs to
circumvent Cargo issue rust-lang/cargo#2589.

By specifying the memchr dependency using its Git URL rather than its
crates.io version, it looks to Cargo different than the memchr
dependency specified by bindgen. Hence the features of both dependencies
are no longer unionized and we get a memchr free of libc.

The plan is to use this workaround only as long as the above Cargo issue
is not fixed. Afterwards we can just use the cstr_core release from
crates.io again.
@tarcieri
Copy link

From what I can tell this is the same bug as #4664 and #4866. Of these, I prefer the writeup in #4866.

@Nemo157
Copy link
Member

Nemo157 commented Jan 27, 2019

Build and dev dependencies are slightly different. You can have a fully independent copy of a build dependency since they are never linked into the same binary. For dev dependencies you might be passing values from that dependency into the crate under test, which requires that you use the same instantiation of the crate. (Basically, it seems to me that the dev-dependency case is slightly more complex, so having a separate issue to discuss it would be useful).

@tarcieri
Copy link

tarcieri commented Jan 27, 2019

@Nemo157 here is a reproduction of a build dependency (bindgen) triggering inclusion of std in a resulting target:

https://github.com/coolreader18/rsspire/blob/weird-std/nspire-sys/Cargo.toml#L10

Here is a reddit thread about the above repro:

https://www.reddit.com/r/rust/comments/a91qv0/for_a_no_std_crate_libstd_is_still_included_in/

In my personal observation many people are encountering this particular issue, especially in regard to tools like bindgen which use several dependencies which have an optional std feature which no_std crates do not want activated in the resulting target:

https://tonyarcieri.com/rust-in-2019-security-maturity-stability#bad-interactions-between-code-classprettyprin_2

@Nemo157
Copy link
Member

Nemo157 commented Jan 27, 2019

@tarcieri I’m referring to the eventual result of fixing these issues, not to the status quo, the implementation for each will be slightly different.

@tarcieri
Copy link

tarcieri commented Jan 27, 2019

I'm looking into what it would take to actually fix this and will post my results on #4866 (my preferred writeup on this issue)

@ehuss
Copy link
Contributor

ehuss commented Feb 24, 2020

Non-unification of build-dependency features has been implemented and is available as a nightly-only feature on the latest nightly 2020-02-23. See the tracking issue at #7915.

If people following this issue could try it out, and leave your feedback on the tracking issue (#7915), I would appreciate it. Particularly we'd like to know if it helps your project, does it cause any breakage, and does it significantly increase initial compile time.

@ehuss ehuss closed this as completed Feb 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-build-dependencies Area: [build-dependencies] A-dependency-resolution Area: dependency resolution and the resolver A-features Area: features — conditional compilation C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted`
Projects
None yet
Development

No branches or pull requests