Expose build.target .cargo/config setting as packages.target in Cargo.toml #9030

Ekleog · 2020-12-31T01:48:50Z

Hey!

I'm trying to do my first cargo contribution by implementing per-crate target settings as per the irlo thread ; and I think I have a draft that looks good-ish (the root units returned by generate_targets have the right kinds set).

Closes #7004

Edit: the below problem description is now solved in the latest version of this PR, please ignore

But for some reason running on a test project now blocks on Blocking waiting for file lock on build directory and I have literally no idea how my changes could trigger this… would anyone have an idea of how the changes could lead to infinitely blocking there? (I already tried cargo clean just in case and it didn't appear to help)

FWIW, the output that looks hopeful to me is, on my testbed workspace:

Root units [out of generate_targets] are [...]:
 - package ‘smtp-client’ with kind ‘Target(CompileTarget { name: "x86_64-unknown-linux-gnu" })’
 - package ‘smtp-server’ with kind ‘Target(CompileTarget { name: "x86_64-unknown-linux-gnu" })’
 - package ‘yuubind-config’ with kind ‘Target(CompileTarget { name: "x86_64-unknown-linux-gnu" })’
 - package ‘smtp-message-fuzz’ with kind ‘Target(CompileTarget { name: "x86_64-unknown-linux-gnu" })’
 - package ‘yuubind-rpc’ with kind ‘Target(CompileTarget { name: "x86_64-unknown-linux-gnu" })’
 - package ‘yuubind-config-example’ with kind ‘Target(CompileTarget { name: "x86_64-unknown-linux-gnu" })’
 - package ‘smtp-message-fuzz’ with kind ‘Target(CompileTarget { name: "x86_64-unknown-linux-gnu" })’
 - package ‘yuubind-config-example’ with kind ‘Target(CompileTarget { name: "wasm32-unknown-unknown" })’
 - package ‘smtp-queue’ with kind ‘Target(CompileTarget { name: "x86_64-unknown-linux-gnu" })’
 - package ‘smtp-message-fuzz’ with kind ‘Target(CompileTarget { name: "x86_64-unknown-linux-gnu" })’
 - package ‘smtp-message’ with kind ‘Target(CompileTarget { name: "x86_64-unknown-linux-gnu" })’
 - package ‘smtp-server-fuzz’ with kind ‘Target(CompileTarget { name: "x86_64-unknown-linux-gnu" })’
 - package ‘yuubind-config’ with kind ‘Target(CompileTarget { name: "wasm32-unknown-unknown" })’
 - package ‘yuubind’ with kind ‘Target(CompileTarget { name: "x86_64-unknown-linux-gnu" })’
 - package ‘smtp-queue-fs’ with kind ‘Target(CompileTarget { name: "x86_64-unknown-linux-gnu" })’

(where both yuubind-config and yuubind-config-example are being configured to be wasm32-unknown-unknown and the other ones stay as host).

Interestingly enough, if I remove the target setting from yuubind-config (and leave it on yuubind-config-example) then it does no longer block on waiting for file lock on build directory, even though it does not actually compile with wasm32-unknown-unknown. And it does appear to correctly build yuubind-config-example as wasm32.

My investigation shows that it appears to happen iff there is a package with package.target being set that has dependencies.

This most likely is a bug in my code (eg. I build only the root units and not the whole unit graph maybe?), and am going to keep investigating it as such, but maybe someone would already know how dependency resolution could interact with build lock acquisition and give me hints?

Anyway, thank you all for all you do cargo!

rust-highfive · 2020-12-31T01:48:53Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @ehuss (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.

Ekleog · 2020-12-31T03:15:35Z

As this PR currently appears to work (for my test repository), I'd like to ask for a first round of review before polishing up all the details that are sure to be problematic.

What do you think about this? As this is my first cargo contribution, maybe there is some obvious place I forgot changing, and I should be doing this before even thinking of adding tests and polishing things up? Also, should I (and if yes is there an example somewhere in the code of how) hide this new option behind an unstable gate?

If things look good enough, I'll be glad to poke around the cargo test framework and try adding tests, and to look for what documentation might need updating.

(BTW, there are already failing CI tests that appear to be unrelated to my changes, like this one, I'm not sure whether I'm expected to drive-by fix them or not)

Eh2406 · 2020-12-31T04:42:32Z

With the holidays in the US we are not paying as much attention as we normally do. For example the unrelated CI failing are something to do with Nightly, and normally there would be a preity prompt PR fixing it (or turning off Nightly testing) that you could rebase onto. Thank you for your contribution and patients.

Tests and documentation would be appreciated. I for one find it a good place to start finding out "what are you trying to do" as a start of a code review.
It is possible that we will make this insta stable, but it is more likely that when someone does a full review we will want a feature flag. Instructions for adding one are at https://github.com/rust-lang/cargo/blob/8917837fb682c6515c1cbfdbd16be95bb64bd6f1/src/cargo/core/features.rs

Eh2406 · 2020-12-31T17:55:17Z

Thanks to @ehuss for #9033 that fixes CI.

Ekleog · 2021-01-01T11:34:31Z

Thank you for your feedback! I have just fixed an issue I noticed while using a patched cargo, and rebased to a master that hopefully has CI fixed.

I also added the documentation, that hopefully should help understanding what this diff is about.

I haven't added tests yet, though, as I'd like to confirm that at least the idea of the change is legit. The irlo thread listed in the top post, AFAIU, says that this setting should be made available, but I'm not sure that eg. [package] is the right place to put it in.

Does this sound reasonable enough? And what do you think about the documented changes, that I hopefully implemented not too awfully here?

Cheers!

Ekleog · 2021-01-01T11:44:26Z

Another thought I just came up with: should package.target = "wasm32-unknown-unknown" imply lib.test = false? What about package.target = "i686-unknown-linux-gnu" when the host is x86_64-unknown-linux-gnu?

alexcrichton

Thanks for the PR! I think it may make sense to land a few of the refactorings in the backend (e.g. storing a list of kinds in the BuildContext) ahead of time before this PR if you'd like.

Otherwise I think it'd be good too add some tests here. Additionally I'm not entirely sure what the final design will be for target and how it interacts with --target, but I added some comments below on that.

alexcrichton · 2021-01-04T16:17:11Z

src/cargo/core/compiler/build_context/target_info.rs

+        let all_kinds = requested_kinds
+            .iter()
+            .copied()
+            .chain(ws.members().flat_map(|p| p.manifest().kind().into_iter()));


I think that this will need to iterate over the entire crate graph instead of just ws.members() to find requested targets?

I've tried thinking about this, but it looks to me like RustcTargetData does not already have information enough to actually list the entire crate graph, and this only happens in BuildContext which gets called after that? And as RustcTargetData also gets used from eg. cargo fetch, which AFAIU does itself not have enough information to iterate over the entire crate graph — and it is indeed using the kind data in RustcTargetData.

Now, seeing as the structures filled here are AFAIU just caches, it'd be very possible to just fill them in on-demand instead, which should solve the issue. Would that sound good to you?

src/cargo/core/compiler/compilation.rs

src/cargo/core/compiler/context/mod.rs

src/cargo/ops/cargo_compile.rs

src/cargo/util/toml/mod.rs

Ekleog · 2021-01-05T01:49:52Z

I think I have noticed another issue of this PR as is, though I still have to actually check it: feature resolution happens only once, and so if a crate is depended on with a different set of features by target = "wasm32-unknown-unknown" and host-target crates, it'll end up with all features enabled for both targets. Which can end up causing issues, when eg. one optional dependency isn't supported on one of the targets.

Fixing this would require changing the feature resolution algorithm, which I expect would be quite a bit more invasive. How do you think we should go? First land the “basic” stuff and then work towards fixing feature resolution while keeping the flag gated? Or finish work on getting the feature resolution first, and only then land it all at once, at the expense of this ending up being a (much?) bigger PR?

Involved in rust-lang#9030

Ekleog · 2021-01-05T02:20:31Z

Uh well, I posted that message faster than I wanted to, sorry for the notification noise.

So I also wanted to say, I've addressed a few of the comments, and replied to a few other comments. Split the refactoring towards #9046, and merged that branch here so it still works (hope git won't merge conflict me at the end, it's the first time I work in a “please don't rebase” scenario, usually I'd have just squash-rebased on top of that other branch)

Note to self about things still remaining:

Fixing the RustcTargetData handling, maybe by filling the cache on-demand? (if that's confirmed to be the way to go)
Feature-gate the two flags
Update src/doc/src/reference/unstable.md instead of the current manifest format page
Add tests
Figure out how to move forward with respect to feature flags that get depended on by multiple targets

Small refactor, adding a list of all kinds to BuildContext Involved in #9030 cc `@alexcrichton`

Ekleog · 2021-01-05T21:28:50Z

(Note: I've just rebased on top of master now that #9046 landed)

src/cargo/core/compiler/build_context/target_info.rs

src/cargo/ops/cargo_compile.rs

Ekleog · 2021-01-14T23:40:12Z

Ok, so with this latest commit I have I think solved all the issues that have been raised, except:

Actually adding kinds after the fact, because I could not think of any case where the full list of kinds is not known by just exploring the workspace yet. This is because AFAIK artifact dependencies are not a thing yet — as soon as they are we'll just have to call .add_kind() at the places where testing shows crashes and things should just work. But I have not been able yet to think of any test case that would hit a situation where a kind only appears outside the workspace, as then the unit kind would AFAIU just be ignored to take its reverse-dep's kind instead.
Handling feature resolution by having one feature resolution per kind, to avoid that a crate that has features F and G, where F is used by wasm32-wasi and G by $host, ends up building both F and G for both wasm32-wasi and $host. (I have a concrete case where this happened, and F cannot build on $host nor G on wasm32-wasi) The solution to this needs to be informed by the current state of implementing artifact dependencies, as artifact dependencies will also influence feature resolution: if A depends on artifacts from B and C, B depends on D with feature F and C on D with feature G, do both B and C build with a D that has features F and G, or do they each get their own D? Depending on the response of this question + the status of the implementation of artifact dependencies, it will be hopefully possible to make progress here without creating merge conflicts between the two PRs — I don't want to cause trouble to other PRs that would currently be in the process of being written :)

Ekleog · 2021-04-10T04:08:22Z

@alexcrichton Could you give another look at this PR? I think I handled your last comments, so hopefully it should be ready to go :) (and sorry for myself taking so long to fix the last comments!)

alexcrichton

Oops sorry this fell under my radar! I've lost a bit of context since I last looked at this, but as-is this looks good to me. I suspect there's at least one or two things that will need to be handled later on, but for now this seems fine. Want to go ahead and file a tracking issue and fill that in in the unstable documentation? With some small updates below I think this should be good to merge.

crates/cargo-test-support/Cargo.toml

src/cargo/core/compiler/build_context/target_info.rs

alexcrichton · 2021-04-23T16:05:19Z

src/cargo/util/toml/mod.rs

+            .default_target
+            .as_ref()
+            .map(|t| CompileTarget::new(&*t))
+            .transpose()? // TODO: anyhow::Context isn't imported yet so I guess .context() isn't the right way to do it?


I think it's safe to remove this TODO?

Hmm, it's true that I had totally forgotten about this TODO, sorry! It was there because I was thinking that there may be a want to add details to the error message, but I have basically no knowledge of the cargo error handling story yet.

I've removed this TODO for now, feel free to let me know if you think there should be an equivalent to .context() here to give a hint to the user that the issue comes from a mis-spelled default_target (if I understood correctly the error conditions of CompileTarget::new) :)

….toml

Ekleog · 2021-04-24T18:40:03Z

Thank you for your feedback! And no problem about this falling under your radar, it fell under mine for a long time before that 😅

The review comments should have been handled, I rebased and fixed a compile error that appeared since the previous rebase, CI passed, and I opened #9406 for the tracking issue. I'm just not clear about the unstable documentation, as AFAIU the rust unstable book is only for rustc and this PR already has changes to something that may be the unstable documentation?

alexcrichton · 2021-04-26T15:00:41Z

Ok thanks! This looks good to me so I'll approve

@bors: r+

For the tracking issue I'll fill in some details.

bors · 2021-04-26T15:00:43Z

📌 Commit fcd7617 has been approved by alexcrichton

bors · 2021-04-26T15:00:52Z

⌛ Testing commit fcd7617 with merge d1c0a9d...

bors · 2021-04-26T15:29:27Z

☀️ Test successful - checks-actions
Approved by: alexcrichton
Pushing d1c0a9d to master...

Update cargo 5 commits in 0ed318d182e465cd66071b91ac3d265af63ef8a1..4369396ce7d270972955d876eaa4954bea56bcd9 2021-04-23 20:54:54 +0000 to 2021-04-27 14:35:53 +0000 - Fix rebuild issues with rustdoc. (rust-lang/cargo#9419) - Always use full metadata hash for -C metadata. (rust-lang/cargo#9418) - Expose build.target .cargo/config setting as packages.target in Cargo.toml (rust-lang/cargo#9030) - Some changes to rustdoc fingerprint checking. (rust-lang/cargo#9404) - Document that CARGO_PKG_ are availble to build.rs (rust-lang/cargo#9405)

Ekleog · 2021-04-30T19:12:01Z

Looks like this landed without issues. Awesome, thank you! ❤️

leap0x7b · 2022-03-02T06:52:30Z

How can I use this?

ewoolsey · 2022-10-05T00:35:05Z

Can someone please explain how to use this feature? I can't find any documentation

weihanglo · 2022-10-05T09:22:18Z

Normally you can find the documentation of an unstable feature from "Unstable Features" chapter in The Cargo Book. The usage is documented there as well.

For more usages, you may also want to take a look at how cargo tests this feature, or keep an eye on the tracking issue to see how people use it.

rust-highfive assigned ehuss Dec 31, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 31, 2020

Ekleog marked this pull request as draft December 31, 2020 01:49

Ekleog marked this pull request as ready for review December 31, 2020 03:09

Ekleog changed the title ~~WIP: Expose build.target .cargo/config setting as packages.target in Cargo.toml~~ Expose build.target .cargo/config setting as packages.target in Cargo.toml Dec 31, 2020

Ekleog mentioned this pull request Dec 31, 2020

Per crate build target in workspace #7004

Closed

Ekleog force-pushed the target-setting branch from 0857df6 to e12e83e Compare January 1, 2021 11:28

Ekleog force-pushed the target-setting branch from a362ea1 to 4ca7980 Compare January 1, 2021 12:11

phil-opp mentioned this pull request Jan 2, 2021

This Month in Rust OSDev (December 2020) rust-osdev/homepage#33

Merged

12 tasks

alexcrichton reviewed Jan 4, 2021

View reviewed changes

Ekleog mentioned this pull request Jan 5, 2021

Small refactor, adding a list of all kinds to BuildContext #9046

Merged

Ekleog added a commit to Ekleog/cargo that referenced this pull request Jan 5, 2021

Small refactor, adding a list of all kinds to BuildContext

2f14f98

Involved in rust-lang#9030

bors added a commit that referenced this pull request Jan 5, 2021

Auto merge of #9046 - Ekleog:store-kinds-list-in-bcx, r=alexcrichton

0583aa4

Small refactor, adding a list of all kinds to BuildContext Involved in #9030 cc `@alexcrichton`

Ekleog force-pushed the target-setting branch from 007019d to b5852fa Compare January 5, 2021 21:28

alexcrichton reviewed Jan 5, 2021

View reviewed changes

src/cargo/core/compiler/build_context/target_info.rs Outdated Show resolved Hide resolved

src/cargo/ops/cargo_compile.rs Show resolved Hide resolved

Ekleog mentioned this pull request Jan 6, 2021

Refactor RustcTargetData to do lazy loading of targets information #9049

Closed

This was referenced Jan 13, 2021

Wrong post-build if .cargo/config contains build target phil-opp/cargo-post#16

Closed

Allow to unset a default target by setting build.target to the empty string #8638

Closed

Add a unstable --ignore-local-config flag #8652

Closed

Ekleog mentioned this pull request Apr 12, 2021

Dependabot can't resolve your Rust dependency files Ekleog/kannader#174

Closed

alexcrichton reviewed Apr 23, 2021

View reviewed changes

Ekleog added 3 commits April 24, 2021 19:12

Expose build.target .cargo/config setting as packages.target in Cargo…

5e11afc

….toml

Improve error message on missing feature gate

2bef76a

handle review comments

731d881

Ekleog force-pushed the target-setting branch from b142bb0 to 731d881 Compare April 24, 2021 17:13

fix build

fcd7617

Ekleog mentioned this pull request Apr 24, 2021

Tracking Issue for per-package-target #9406

Open

3 tasks

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 26, 2021

bors merged commit d1c0a9d into rust-lang:master Apr 26, 2021

bors mentioned this pull request Apr 26, 2021

Configure hosts separately from targets when --target is specified. #9322

Merged

ehuss mentioned this pull request Apr 27, 2021

Update cargo rust-lang/rust#84635

Merged

jameshilliard added a commit to jameshilliard/cargo that referenced this pull request May 1, 2021

Fix rust-lang#9030 rebase.

62e2fd4

phil-opp mentioned this pull request May 3, 2021

The new per-package-target feature does not work with -Zbuild-std #9451

Open

phil-opp mentioned this pull request Nov 14, 2021

Open source composition of Edition 3 phil-opp/blog_os#1063

Open

ehuss added this to the 1.53.0 milestone Feb 6, 2022

chemicstry mentioned this pull request Feb 11, 2022

Specifying build target fornwall/rust-script#23

Closed

mraerino mentioned this pull request Sep 10, 2023

Ensure cargo check works for the workspace without flags r9os/r9#26

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expose build.target .cargo/config setting as packages.target in Cargo.toml #9030

Expose build.target .cargo/config setting as packages.target in Cargo.toml #9030

Ekleog commented Dec 31, 2020 •

edited

Loading

rust-highfive commented Dec 31, 2020

Ekleog commented Dec 31, 2020

Eh2406 commented Dec 31, 2020

Eh2406 commented Dec 31, 2020

Ekleog commented Jan 1, 2021

Ekleog commented Jan 1, 2021

alexcrichton left a comment

alexcrichton Jan 4, 2021

Ekleog Jan 5, 2021

Ekleog commented Jan 5, 2021

Ekleog commented Jan 5, 2021 •

edited

Loading

Ekleog commented Jan 5, 2021

Ekleog commented Jan 14, 2021

Ekleog commented Apr 10, 2021 •

edited

Loading

alexcrichton left a comment

alexcrichton Apr 23, 2021

Ekleog Apr 24, 2021

Ekleog commented Apr 24, 2021 •

edited

Loading

alexcrichton commented Apr 26, 2021

bors commented Apr 26, 2021

bors commented Apr 26, 2021

bors commented Apr 26, 2021

Ekleog commented Apr 30, 2021

leap0x7b commented Mar 2, 2022

ewoolsey commented Oct 5, 2022

weihanglo commented Oct 5, 2022

Expose build.target .cargo/config setting as packages.target in Cargo.toml #9030

Expose build.target .cargo/config setting as packages.target in Cargo.toml #9030

Conversation

Ekleog commented Dec 31, 2020 • edited Loading

rust-highfive commented Dec 31, 2020

Ekleog commented Dec 31, 2020

Eh2406 commented Dec 31, 2020

Eh2406 commented Dec 31, 2020

Ekleog commented Jan 1, 2021

Ekleog commented Jan 1, 2021

alexcrichton left a comment

Choose a reason for hiding this comment

alexcrichton Jan 4, 2021

Choose a reason for hiding this comment

Ekleog Jan 5, 2021

Choose a reason for hiding this comment

Ekleog commented Jan 5, 2021

Ekleog commented Jan 5, 2021 • edited Loading

Ekleog commented Jan 5, 2021

Ekleog commented Jan 14, 2021

Ekleog commented Apr 10, 2021 • edited Loading

alexcrichton left a comment

Choose a reason for hiding this comment

alexcrichton Apr 23, 2021

Choose a reason for hiding this comment

Ekleog Apr 24, 2021

Choose a reason for hiding this comment

Ekleog commented Apr 24, 2021 • edited Loading

alexcrichton commented Apr 26, 2021

bors commented Apr 26, 2021

bors commented Apr 26, 2021

bors commented Apr 26, 2021

Ekleog commented Apr 30, 2021

leap0x7b commented Mar 2, 2022

ewoolsey commented Oct 5, 2022

weihanglo commented Oct 5, 2022

Ekleog commented Dec 31, 2020 •

edited

Loading

Ekleog commented Jan 5, 2021 •

edited

Loading

Ekleog commented Apr 10, 2021 •

edited

Loading

Ekleog commented Apr 24, 2021 •

edited

Loading