Allow cfg(...)ing targets in Cargo.toml #5367

fitzgen · 2018-04-16T21:24:14Z

This is a WORK IN PROGRESS!

Currently, [[bin]]s do the Right Thing (I think) and maybe [lib] too but I haven't tested. Mostly posting this to acquire some early feedback. cc @alexcrichton

Fix #4900

Sort of fix #4881 (it seems like maybe cfg'ing the [lib] is good enough that we don't need to be able to cfg the individual crate-types?)

rust-highfive · 2018-04-16T21:24:17Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @matklad (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

matklad · 2018-04-17T11:43:54Z

I feed a bit uneasy about cfging the libraries specifically... Looks like registry and crates.io might want to about "platform-specific libraries" somehow?

fitzgen · 2018-04-17T17:05:38Z

My goal is for cargo test and cargo bench to work with crates primarily targeting WebAssembly without needing to change any source, so long as those tests and benches don't rely on wasm imports or anything like that. Right now, to get this behavior, you have to manually split your crate into a core library crate and a wasm API crate, and I find this to be an unnecessary paper cut.

matklad · 2018-04-17T17:17:46Z

Yeah, I think cfg would be fine for tests and benches, it's only the lib which makes me feel uneasy... OTOH, supporting cfg only on some crate types seems arbitrary...

alexcrichton

Thanks so much @fitzgen, this is looking pretty good! I definitely feel like this is on the right track. I think that this is also highly related to #5335 (as a heads up).

@matklad I don't think this'll affect the index/crates.io b/c there's nothing that affects dependency resolution but rather just how crates are compiled, right?

alexcrichton · 2018-04-17T16:03:00Z

src/cargo/util/toml/mod.rs

@@ -556,6 +556,9 @@ impl TomlManifest {
                                        .or_else(|| v.build_dependencies2.as_ref()),
                                )?,
                                build_dependencies2: None,
+                                lib: v.lib.clone(),
+                                bin: v.bin.clone(),
+                                example: v.example.clone(),


Could test also be included here? And bench?

matklad · 2018-04-19T10:17:47Z

I don't think this'll affect the index/crates.io b/c there's nothing that affects dependency resolution but rather just how crates are compiled, right?

Hm, probably. I was thinking that maybe this should affect dependency resolution, so that you get error "crate foo is unsupported for $host_triple" earlier, during lockfile generation, but looks like this is not possible because you know $host_triple only when you are actually compiling. I think this clears my doubts, so removing the nomination.

bors · 2018-04-21T16:33:29Z

☔ The latest upstream changes (presumably #5335) made this pull request unmergeable. Please resolve the merge conflicts.

fitzgen · 2018-04-27T22:11:13Z

I think this is ready for another look: r? @alexcrichton

bors · 2018-04-27T23:49:28Z

☔ The latest upstream changes (presumably #5384) made this pull request unmergeable. Please resolve the merge conflicts.

alexcrichton · 2018-04-29T22:49:59Z

src/cargo/util/toml/targets.rs

+                            .flat_map(|benches| {
+                                benches.iter()
+                                    .cloned()
+                                    .filter_map(|bench| {


This is a particularly gnarly super-nested loop duplicated (it looks like) in a few places, could these be tightened up?

alexcrichton · 2018-04-29T22:50:40Z

tests/testsuite/cfg.rs

+    assert_that(
+        p.cargo("build").arg("-v"),
+        execs().with_status(0)
+            // TODO FITZGEN: I don't know why this assertion fails...


You'll want to write down autobins = true in the manifest above to get this working.

alexcrichton

Looking good! Could some tests also be added exhibiting "layering" configuration where a unit test or something like that is configured in multiple [[test]] blocks (some platform-specific).

Additionally could a test be added to ensure that you can't define more than one [lib] target?

chrisballinger · 2018-05-02T18:42:00Z

Apologies if this should be brought up in another issue, but I'm curious if this PR would this allow you to do something like this?

# Cargo.toml
[lib]
[target.'cfg(target_os = "ios")']
crate-type = ["staticlib"]
[target.'cfg(target_os = "android")']
crate-type = ["cdylib"]

I'm trying to make a "universal" lib crate that has feature gates for two platform specific sub-crates. For example:

[workspace]
members = ["Source/Rust/generic", "Source/Rust/apple", "Source/Rust/java"]

[package]
name = "example-universal"
version = "0.1.0"
authors = ["Chris Ballinger <[email protected]>"]

[lib]
name = "example"
path = "Source/Rust/universal/lib.rs"
crate-type = ["staticlib", "cdylib"]

[features]
apple = ["example-apple"]
java = ["example-java"]

[profile.release]
debug = true

[dependencies]
example-generic = { path = "Source/Rust/generic" }
example-apple = { path = "Source/Rust/apple", optional = true }
example-java = { path = "Source/Rust/java", optional = true }

[package]
name = "example-generic"
version = "0.1.0"
authors = ["Chris Ballinger <[email protected]>"]

[lib]
name = "example_generic"

[dependencies]
libc = "0.2"

[package]
name = "example-apple"
version = "0.1.0"
authors = ["Chris Ballinger <[email protected]>"]

[lib]
name = "example_apple"
crate-type = ["staticlib"]

[dependencies]
dispatch = "0.1"
objc-foundation = "0.1"
example-generic = { path = "../generic" }

[package]
name = "example-java"
version = "0.1.0"
authors = ["Chris Ballinger <[email protected]>"]

[lib]
name = "example_java"
crate-type = ["cdylib"]

[dependencies]
jni = "0.10"
example-generic = { path = "../generic" }

The issue here is that in my "universal" parent crate, cdylib isn't a valid crate-type for ios, and staticlib isn't valid for android. When compiling for iOS with cargo-lipo, cargo gives warnings about unsupported cdylib:

$ cargo lipo --features "apple"
   Compiling block v0.1.6
   Compiling dispatch v0.1.2
   Compiling malloc_buf v0.0.6
   Compiling objc v0.2.2
   Compiling objc_id v0.1.0
   Compiling objc-foundation v0.1.1
   Compiling example-apple v0.1.0 (file:///rust-universal-template/Source/Rust/apple)
   Compiling example-universal v0.1.0 (file:///rust-universal-template)
warning: dropping unsupported crate type `cdylib` for target `aarch64-apple-ios`

    Finished dev [unoptimized + debuginfo] target(s) in 2.13 secs
   Compiling block v0.1.6
   Compiling dispatch v0.1.2
   Compiling malloc_buf v0.0.6
   Compiling objc v0.2.2
   Compiling objc_id v0.1.0
   Compiling objc-foundation v0.1.1
   Compiling example-apple v0.1.0 (file:///rust-universal-template/Source/Rust/apple)
   Compiling example-universal v0.1.0 (file:///rust-universal-template)
warning: dropping unsupported crate type `cdylib` for target `armv7-apple-ios`

    Finished dev [unoptimized + debuginfo] target(s) in 2.12 secs
   Compiling block v0.1.6
   Compiling dispatch v0.1.2
   Compiling malloc_buf v0.0.6
   Compiling objc v0.2.2
   Compiling objc_id v0.1.0
   Compiling objc-foundation v0.1.1
   Compiling example-apple v0.1.0 (file:///rust-universal-template/Source/Rust/apple)
   Compiling example-universal v0.1.0 (file:///rust-universal-template)
warning: dropping unsupported crate type `cdylib` for target `i386-apple-ios`

    Finished dev [unoptimized + debuginfo] target(s) in 2.15 secs
   Compiling block v0.1.6
   Compiling dispatch v0.1.2
   Compiling malloc_buf v0.0.6
   Compiling objc v0.2.2
   Compiling objc_id v0.1.0
   Compiling objc-foundation v0.1.1
   Compiling example-apple v0.1.0 (file:///rust-universal-template/Source/Rust/apple)
   Compiling example-universal v0.1.0 (file:///rust-universal-template)
warning: dropping unsupported crate type `cdylib` for target `x86_64-apple-ios`

fitzgen · 2018-05-02T19:05:08Z

@chrisballinger, with this PR, you will be able to do

[target.'cfg(not(target = "x86_64-apple-ios"))'.lib]
name = "example"
path = "Source/Rust/universal/lib.rs"
crate-type = ["staticlib", "cdylib"]

[target.'cfg(target = "x86_64-apple-ios")'.lib]
name = "example"
path = "Source/Rust/universal/lib.rs"
crate-type = ["staticlib"]

fitzgen · 2018-05-02T20:59:14Z

Ok, rebased. Will address review feedback now.

fitzgen · 2018-05-02T23:05:38Z

Enforcing only a single [lib] is tricky...

We need a BuildContext in order to determine wether any given cfg(...) is activated or not
But we don't have one of those until much later, after we have collected all the targets, eg in compile_ws
So I figured I could do the checking in compile_ws
However, by this time we've lost the information about which crate a given [lib] is for, so enforcing a single [lib] gets tripped up by the fact that there are different [lib]s from different crates
So I tried to add a PackageId member to TargetKind::Lib, so that we can always determine what crate a [lib] is originally from
But everywhere we call Target::lib_target, which constructs TargetKind::Lib, we don't have a package yet, and therefore don't have a PackageId

Does anyone have any suggestions on how to move forward?

fitzgen · 2018-05-02T23:08:50Z

Aha! I just figured out a way to pass a package id through!

fitzgen · 2018-05-02T23:43:23Z

Ok, so in the latest commit I am ensuring that there is only one [lib] target per (PackageId, Profile) pair. This passes all the cfg(..) tests I've written, but fails on a couple of the existing tests, particularly ones involving --all-targets.

Does anyone have any suggestions?

fitzgen · 2018-05-02T23:45:19Z

src/cargo/ops/cargo_compile.rs

+            })
+            .collect::<Vec<_>>();
+
+        // Ensure that there is only one [lib] activated per crate.


This is where the unique [lib] checking happens, fwiw.

matklad · 2018-05-03T11:34:40Z

src/cargo/ops/cargo_compile.rs

+            let mut pkg_libs: HashMap<(&PackageId, &str), Vec<&Unit>> = Default::default();
+
+            for unit in &units {
+                if let TargetKind::Lib(ref pkgid, _) = *unit.target.kind() {


Hm, if I understand correctly, you don't have to add pkgid to TargetKind, because you can use unit.pkg.package_id()?

Thanks, I didn't see that.

matklad · 2018-05-03T11:40:27Z

tests/testsuite/cfg.rs

+
+            [target.'cfg(not(target_pointer_width = "1"))'.lib]
+            name = "always"
+            path = "src/always.rs"


I feel uneasy about the fact that you specify different names depending on the platform.... Should we make this case an error perhaps?

As mentioned below, if they both activate, then it will be an error. No merging is done.

alexcrichton · 2018-05-03T14:51:05Z

The updates are looking pretty good with @matklad's thoughts as well, but it may be good to discuss the overall interface we'd like here as well.

I believe we don't want this to enable multiple [lib] targets for a crate, so we definitely want to restrict it to still "one at a time". How do we want this to all interact though?

For example let's take something like:

[target.foo.lib]
name = 'bar'

This already implies there may be two targets in the manifest. If src/lib.rs exists then there's one that we can auto-infer but there's also one explicitly specified. Does this mean that there's two? Does Cargo avoid auto-inferring the first one? Can we configure Cargo's auto-inference behavior? (this may just mean we want something like autolib = true in the manifest like we have autobins = false today, added recently).

The next question would be how do these targets interact? For example if don't automatically infer a target then it means the library is not workable by default on non-foo platforms (as no [lib] exists). To fix this you'd need to do [target.cfg(not(foo)).lib]. If we do automatically infer [lib] then this wouldn't work by default, right? (as there would be two [lib] targets by default).

Or do we want a sort of "merging" of targets? For example what about:

[target.'cfg(target_arch = "wasm32")'.lib]
crate-type = ["cdylib"]

Does this "merge" with the main [lib] profile when activated? Or add a new profile which is separately configure? (if merge then we also need to figure out what it means to merge!)

I feel like merging is probably what one might expect, but it's something we'll want to think through as well as consider what it means for other targets as well? (can you "merge" two test targets?)

Perhaps we can just say that any targets with the same name, inferred or explicitly specified, get merged (and we specify how they're merged). I think that'd probably cover the issues here?

chrisballinger · 2018-05-03T15:44:02Z

@fitzgen Awesome, thank you!

fitzgen · 2018-05-03T17:56:18Z

I was assuming that we would not do merging, and that if we ever ended up in a situation where two [lib]s were activated at the same time, then we would bail out and report an error.

alexcrichton · 2018-05-04T14:24:29Z

@fitzgen hm so if we don't do any merging, how's this supposed to be used? That means if I have something like:

[lib]
test = false

[target.'cfg(target_arch = "wasm32")'.lib]
crate-type = ["cdylib"]
test = false

that won't work on wasm, right? (because on wasm there's two lib targets)

fitzgen · 2018-05-04T15:09:11Z

In that case the check for a unique lib would trigger and there would be an error. You would have to have a cfg for wasm and a cfg for not wasm.

I agree that this isn't super ergonomic, but it seems that we can always loosen the restrictions and start merging later. I just want to get the minimum thing working for now.

alexcrichton · 2018-05-04T23:37:04Z

Ok that makes sense yeah, I agree that this is the conservative route and we can always soup it up later!

That reminds me though, can you be sure this functionality is behind a Cargo feature?

bors · 2018-05-05T19:05:56Z

☔ The latest upstream changes (presumably #5358) made this pull request unmergeable. Please resolve the merge conflicts.

fitzgen · 2018-05-07T20:54:24Z

@alexcrichton

That reminds me though, can you be sure this functionality is behind a Cargo feature?

Like a normal feature in Cargo.toml? Are you intending for this to be available only to folks who build their own cargo? Is there someway we can automatically make it nightly-only?

alexcrichton · 2018-05-08T14:23:16Z

@fitzgen oh there's a src/cargo/core/features.rs which allows defining nightly features for Cargo (not a cargo feature for Cargo), it just requires that you have a nightly Cargo and write down in the manifest that you're using a nightly feature (similar as for nightly features in rustc)

Fix rust-lang#4900 Sort of addresses rust-lang#4881 as well.

fitzgen · 2018-05-22T18:41:22Z

The original motivation behind this PR was to allow #[test]s and #[bench]es to live in the same crate as a crate that was targeting wasm. Somehow, this seems possible now, and without all the cfg(...)ing that this PR requires.

$ tree -I target
.
├── benches
│   └── bench.rs
├── Cargo.lock
├── Cargo.toml
├── src
│   └── lib.rs
└── tests
    └── tests.rs

3 directories, 5 files

$ cat src/lib.rs 
#[no_mangle]
pub extern "C" fn exported_wasm_function() -> u32 {
    42
}

#[cfg(test)]
mod tests {
    #[test]
    fn test_in_same_crate_should_work() {
        assert_eq!(2 + 2, 4);
    }
}

$ cat tests/tests.rs 
extern crate rustwasmtest;

#[test]
fn test_in_tests_dir_should_work() {
    assert_eq!(rustwasmtest::exported_wasm_function(), 42);
}

$ cat benches/bench.rs 
#![feature(test)]

extern crate test;
extern crate rustwasmtest;

#[bench]
fn benches_in_benches_dir_should_work(b: &mut test::Bencher) {
    b.iter(|| rustwasmtest::exported_wasm_function());
}

Tests work:

$ cargo test
    Finished dev [unoptimized + debuginfo] target(s) in 0.01s
     Running target/debug/deps/rustwasmtest-7521e305e328bd1a

running 1 test
test tests::test_in_same_crate_should_work ... ok

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out

     Running target/debug/deps/tests-3c76b8a78c7fad89

running 1 test
test test_in_tests_dir_should_work ... ok

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out

   Doc-tests rustwasmtest

running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out

Benches work:

$ cargo bench
   Compiling rustwasmtest v0.1.0 (file:///tmp/rustwasmtest)
    Finished release [optimized] target(s) in 0.79s
     Running target/release/deps/rustwasmtest-6435d7443081b84e

running 1 test
test tests::test_in_same_crate_should_work ... ignored

test result: ok. 0 passed; 0 failed; 1 ignored; 0 measured; 0 filtered out

     Running target/release/deps/bench-0cdba329b98a4b6a

running 1 test
test benches_in_benches_dir_should_work ... bench:           1 ns/iter (+/- 0)

test result: ok. 0 passed; 0 failed; 0 ignored; 1 measured; 0 filtered out

And compiling to wasm still works:

$ cargo build --target wasm32-unknown-unknown
    Finished dev [unoptimized + debuginfo] target(s) in 0.01s

Given all that, my mission here is done, and I am going to close this PR.

rust-highfive assigned matklad Apr 16, 2018

dwijnand mentioned this pull request Apr 16, 2018

link cargo to its CONTRIBUTING.md rust-lang/highfive#132

Merged

matklad added the I-nominated-to-discuss To be discussed during issue triage on the next Cargo team meeting label Apr 17, 2018

alexcrichton reviewed Apr 17, 2018

View reviewed changes

alexcrichton mentioned this pull request Apr 17, 2018

Adding #[wasm_bindgen] means you can't run cargo bench anymore rustwasm/wasm-bindgen#114

Closed

matklad removed the I-nominated-to-discuss To be discussed during issue triage on the next Cargo team meeting label Apr 19, 2018

fitzgen force-pushed the cfg-for-targets branch 6 times, most recently from a20c633 to f8d27ce Compare April 27, 2018 22:10

rust-highfive assigned alexcrichton and unassigned matklad Apr 27, 2018

fitzgen changed the title ~~[WIP] allow cfg(...)ing targets in Cargo.toml~~ Allow cfg(...)ing targets in Cargo.toml Apr 27, 2018

alexcrichton reviewed Apr 29, 2018

View reviewed changes

fitzgen force-pushed the cfg-for-targets branch 2 times, most recently from 29c37b6 to 19787f5 Compare May 2, 2018 20:54

fitzgen force-pushed the cfg-for-targets branch from 19787f5 to 505a7ad Compare May 2, 2018 21:30

fitzgen force-pushed the cfg-for-targets branch from 505a7ad to 81faca3 Compare May 2, 2018 23:41

fitzgen commented May 2, 2018

View reviewed changes

matklad reviewed May 3, 2018

View reviewed changes

fitzgen force-pushed the cfg-for-targets branch 2 times, most recently from ef093d7 to 6ecd1df Compare May 7, 2018 21:13

Allow toml targets to be conditionally compiled with cfg(..)

2f2b827

Fix rust-lang#4900 Sort of addresses rust-lang#4881 as well.

fitzgen force-pushed the cfg-for-targets branch from 6ecd1df to 2f2b827 Compare May 8, 2018 22:33

fitzgen closed this May 22, 2018

faisal00813 mentioned this pull request Jun 27, 2018

Android builds arm arm64 and x86 hyperledger-archives/indy-sdk#777

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow cfg(...)ing targets in Cargo.toml #5367

Allow cfg(...)ing targets in Cargo.toml #5367

fitzgen commented Apr 16, 2018 •

edited

Loading

rust-highfive commented Apr 16, 2018

matklad commented Apr 17, 2018

fitzgen commented Apr 17, 2018

matklad commented Apr 17, 2018

alexcrichton left a comment

alexcrichton Apr 17, 2018

matklad commented Apr 19, 2018

bors commented Apr 21, 2018

fitzgen commented Apr 27, 2018

bors commented Apr 27, 2018

alexcrichton Apr 29, 2018

alexcrichton Apr 29, 2018

alexcrichton left a comment

chrisballinger commented May 2, 2018 •

edited

Loading

fitzgen commented May 2, 2018

fitzgen commented May 2, 2018

fitzgen commented May 2, 2018

fitzgen commented May 2, 2018

fitzgen commented May 2, 2018

fitzgen May 2, 2018

matklad May 3, 2018

fitzgen May 7, 2018

matklad May 3, 2018

fitzgen May 7, 2018

alexcrichton commented May 3, 2018

chrisballinger commented May 3, 2018

fitzgen commented May 3, 2018

alexcrichton commented May 4, 2018

fitzgen commented May 4, 2018

alexcrichton commented May 4, 2018

bors commented May 5, 2018

fitzgen commented May 7, 2018

alexcrichton commented May 8, 2018

fitzgen commented May 22, 2018

Allow cfg(...)ing targets in Cargo.toml #5367

Allow cfg(...)ing targets in Cargo.toml #5367

Conversation

fitzgen commented Apr 16, 2018 • edited Loading

rust-highfive commented Apr 16, 2018

matklad commented Apr 17, 2018

fitzgen commented Apr 17, 2018

matklad commented Apr 17, 2018

alexcrichton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matklad commented Apr 19, 2018

bors commented Apr 21, 2018

fitzgen commented Apr 27, 2018

bors commented Apr 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexcrichton left a comment

Choose a reason for hiding this comment

chrisballinger commented May 2, 2018 • edited Loading

fitzgen commented May 2, 2018

fitzgen commented May 2, 2018

fitzgen commented May 2, 2018

fitzgen commented May 2, 2018

fitzgen commented May 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexcrichton commented May 3, 2018

chrisballinger commented May 3, 2018

fitzgen commented May 3, 2018

alexcrichton commented May 4, 2018

fitzgen commented May 4, 2018

alexcrichton commented May 4, 2018

bors commented May 5, 2018

fitzgen commented May 7, 2018

alexcrichton commented May 8, 2018

fitzgen commented May 22, 2018

fitzgen commented Apr 16, 2018 •

edited

Loading

chrisballinger commented May 2, 2018 •

edited

Loading