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

Need ability to add dependencies based on #[cfg()] #1007

Closed
codyps opened this issue Dec 3, 2014 · 20 comments
Closed

Need ability to add dependencies based on #[cfg()] #1007

codyps opened this issue Dec 3, 2014 · 20 comments
Labels
P-high Priority: High

Comments

@codyps
Copy link
Contributor

codyps commented Dec 3, 2014

For example, in curl-rust:

Usage: https://github.com/carllerche/curl-rust/blob/5d0f5c8848e3cf1e12480a1923ae888e24d58f63/src/lib.rs#L10
Dependence: https://github.com/carllerche/curl-rust/blob/5d0f5c8848e3cf1e12480a1923ae888e24d58f63/curl-sys/Cargo.toml#L18-L30

Also see alexcrichton/curl-rust#30

In summary: without this, we end up with ad-hoc lists of triples in various Cargo.toml files, which breaks the build for targets with a non-matching triple that do match #[cfg(...)].

@alexcrichton
Copy link
Member

To clarify, one may wish to say [target.unix.dependencies] and [target.windows.dependencies] instead of listing out triples. This will help correspond to cfg(unix) and cfg(windows) in the compiler but requires duplication unfortunately.

@carllerche
Copy link
Member

👍 (sorry for the useless comment, but its the only way to search for issues!)

@alexcrichton
Copy link
Member

Another alternative mentioned by @larsbergstrom would be:

[target."x86_64-*".dependencies]
# ...

[target."*-linux-*".dependencies]
# ...

@codyps
Copy link
Contributor Author

codyps commented May 26, 2015

I'd like to caution against trying to assume anything about targets from their triples.
Rustc currently just treats targets as arbitrary strings: their contents is entirely up to whomever is adding the target.

Trying to extract meaning about targets out of the triple will be fairly error prone & in some cases will completely fail (or be impossible).

Possibly: rustc would expose a way of determining the attributes of a given target (looking at it's builtin targets & the requested json target) and cargo would query that.

Alternately: we could remove all the builtin target knowledge and push it into json files installed in rustlib, and cargo could read them directly.

chris-morgan added a commit to chris-morgan/term that referenced this issue Jul 30, 2015
`kernel32` and `winapi` are completely pointless for non-Windows builds.
(They’re empty crates on non-Windows owing to a `#![cfg(windows)]` crate
attribute so that they build.)
They shouldn’t even be depended on for non-Windows builds.

The duplication is ugly, but necessary for now; see also
rust-lang/cargo#1007
@metajack
Copy link

I'd like to push on this feature a little bit. After dealing with this in Servo and living with having to specify the same dependency multiple times (2x for linux and 2x for arm!), we now have a dependency on rand, which would have to have 4 target triples listed for windows. The upstream author is unwilling to do this, so we're back to conditional compilation of win32api on Linux and OS X.

If this doesn't get fixed soon, the community norm is going to be do make builds inefficient by using conditional compilation, and it's going to be confusing to people who are watching platform specific deps scroll by the build window.

@carllerche
Copy link
Member

@metajack I agree. I am already depending on the windows crate unconditionally in mio because of this. It has confused people but I really don't want to manage huge lists of repeated deps

@alexcrichton
Copy link
Member

@metajack, @carllerche

I agree this is a problem that needs to be fixed, but I don't understand your concerns about unconditionally including crates. The compile time for a crate which is entirely #[cfg]'d away should be nothing and otherwise it should have basically no impact at all. If you have target-specific dependencies then you already have conditional compilation somewhere.

I agree that the ergonomics of target-specific dependencies aren't great today and that including Windows crates on a Unix build "feels wrong", but from a technical perspective I don't think there are many downsides of just unconditionally depending on the Windows crates on Unix.

@metajack
Copy link

metajack commented Aug 3, 2015

My concern is that when you are debugging the Servo build and trying to isolate problems, having 30 extra dependencies to sort through makes your life harder.

Switching this later could potentially leave a slew of crates which #[cfg] out the whole crate all the time and mean adoption of target dependencies will never take off. And for many of Servo's crates, this added a lot of annoying things since every lang item in lib.rs needs to be cfged out; you can't just have a crate level one (or at least you couldn't back when I originally did this for Servo). See servo/io-surface-rs@fba38ef for an example cleanup.

Also, everyone keeps telling me that this free, and yet everyone seems to ignore that Servo has probably the largest dependency graph and surfaces a lot of performance problems that other projects don't see. This will not always be the case; I assume there will be lots of large Rust projects. The fact that people still write build systems to eke out better build times in C and C++ gives some reason to believe that there will be demand to shave these wasted cycles.

@carllerche
Copy link
Member

@alexcrichton the main issue is that it confuses people who are new as it is pretty unexpected behavior.

@alexcrichton
Copy link
Member

@metajack

My concern is that when you are debugging the Servo build and trying to isolate problems, having 30 extra dependencies to sort through makes your life harder.

Isn't this just inherent problems in a project of Servo's size? If you take out all non-relevant dependencies, aren't you still working with ~100 dependencies?

And for many of Servo's crates, this added a lot of annoying things since every lang item in lib.rs needs to be cfged out

By "lang item" do you mean just normal item? This has actually since been fixed in the compiler so you can just place #![cfg(target_os = "macos")] at the top of the crate and then the entire crate is omitted whenever you compile on something other than OSX. An example of this is winapi-rs.

Also, everyone keeps telling me that this free, and yet everyone seems to ignore that Servo has probably the largest dependency graph and surfaces a lot of performance problems that other projects don't see. This will not always be the case; I assume there will be lots of large Rust projects.

Right I don't mean to say that it's literally free, just that in the time it takes to compile the script crate I think that hundreds of empty crates could possibly be compiled. I don't think that have a few extra leaf dependencies (even up to 30) would add much to a build time. That being said I want to clarify if you've measured something to the contrary in this regard.

Overall I don't mind putting more effort into solving this, but I just want to make sure that it's not being prioritized because it's a "nice to have" and instead has some solid technical motivation. If Servo does indeed see an noticeable improvement in build times or general "Cargo is doing things" times then that's a pretty solid reason.


@carllerche

the main issue is that it confuses people who are new as it is pretty unexpected behavior.

Sure, I can totally understand that, just wanted to clarify if there was a technical reason for wanting to fast-track this.

@metajack
Copy link

metajack commented Aug 3, 2015

Fair points. The reasons are clearly mostly social ones. Platform deps are too difficult now, and I feel that if we don't make them usable soon (especially for Windows) then we risk them not getting community buy in.

I'm glad to hear that you can use #![cfg()] at a crate level now.

@alexcrichton
Copy link
Member

Yeah I'm also worried about community buy in, but thanks for the clarifications!

@jethrogb
Copy link
Contributor

This should be easy to implement using rustc_back::target::Target, but I guess using unstable features in Cargo is a no-go?

#![feature(rustc_private)]
extern crate rustc_back;

use rustc_back::target::Target;

fn main() {
    println!("{:?}",Target::search("x86_64-unknown-linux-musl"));
}

@lambda-fairy
Copy link
Contributor

I think we can solve this by adding a --cfg-matches option to rustc.

rustc --cfg-matches EXPR will evaluate EXPR as if it were a #[cfg(..)] expression. It will return 0 if it matches, or 1 if it doesn't match.

Then in Cargo, something like

[target.cfg.unix.dependencies]
# ...

will trigger a call to rustc --cfg-matches unix, adding the dependencies if the call succeeds.

lambda-fairy added a commit to lambda-fairy/rust-errno that referenced this issue Sep 1, 2015
@jpernst
Copy link

jpernst commented Sep 9, 2015

I'd also like to voice support for this feature as it pertains to private production deployments. Even if a dependency happens to compile away to nothing, we still become responsible for including it in our build environment and are exposed to transitive breakage if it ever fails to build for any reason. While Rust itself may be providing strong stability guarantees, libraries on crates.io are under no such obligation. I'm concerned that the status quo is becoming the blanket inclusion of dependencies that aren't really necessary for the core functioning of the libraries that include them. This adds noise to the dependency graph and complicates the evaluation of a library for use in production.

@mvdnes
Copy link
Contributor

mvdnes commented Sep 24, 2015

I would also like a feature like this.

If the cfg or "-linux-" matching is not desirable, maybe we could also let the target be specified in the dependency, rather than the other way around:

[dependencies.winapi]
target = ['i686-pc-windows-gnu', 'i686-pc-windows-msvc', 'x86_64-pc-windows-gnu', 'x86_64-pc-windows-msvc']

This way the version and path and other data would only have to be specified once. In some cases this would seem to make more sense than the other way around.

@retep998
Copy link
Member

@mvdnes That still suffers the issue where if a new windows target triple is working, all the libraries using that form won't work on the new triple until they're updated.

@alexcrichton
Copy link
Member

I've now opened an RFC to help close this issue.

@SimonSapin
Copy link
Contributor

Here is an example where we’d like to reduce duplication somehow:

servo/skia@8678fb6486e#diff-80398c5faae3c069e4e6aa2ed11b28c0R30

@alexcrichton
Copy link
Member

Oh, right! Implemented in #2328

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P-high Priority: High
Projects
None yet
Development

No branches or pull requests

10 participants