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

Allow crates to opt-in to building a single target #632

Merged
merged 21 commits into from Mar 14, 2020

Conversation

@jyn514
Copy link
Member

jyn514 commented Mar 7, 2020

Based off of #532, but backwards compatible.

Closes #627.

Motivation: If crates are only built on one target, I'd be much more comfortable raising limits on a case-by-case basis (e.g. #608, #616). I would also be more comfortable adding more targets (i.e. not tier 1: #563), since specifying those targets would require setting extra-targets and thereby opting out of all others.

Waiting on me to test that this actually works since we don't have unit tests for builds. Done.

  • Can build from a clean cache (no .rustwide-docker directory)
  • Can add essential files (build add-essential-files)
  • Does not pass --target for proc-macros (and in general, for the default build unless default-target is explicitly set) (build crate rstest 0.4.0, testing for #422)
  • Builds for cross-compiled targets work
  • Builds for host target work
  • targets behaves as documented (I only tested targets = ["x86_64-pc-windows-msvc"] and unset default-target; others are tested with unit tests)
  • Passing a duplicate default-target still only builds the default target once
@jyn514

This comment was marked as outdated.

Copy link
Member Author

jyn514 commented Mar 7, 2020

Tested:

  • Building crates without extra-targets keeps the current behavior (all tier 1 targets)
  • Building crates with extra-targets = [] only builds on the default target
  • Building crates with extra-targets = [target] builds on that target as well as the default target
  • Building crates with extra-targets = [default], i.e. adding the default target to extra targets, is a no-op (same as extra-targets = [])
@jyn514 jyn514 changed the title [WIP] Allow crates to opt-in to building a single target Allow crates to opt-in to building a single target Mar 7, 2020
@jyn514

This comment has been minimized.

Copy link
Member Author

jyn514 commented Mar 7, 2020

src/docbuilder/metadata.rs Outdated Show resolved Hide resolved
src/docbuilder/rustwide_builder.rs Outdated Show resolved Hide resolved
src/docbuilder/rustwide_builder.rs Outdated Show resolved Hide resolved
@@ -111,6 +120,8 @@ impl Metadata {
.and_then(|v| v.as_bool()).unwrap_or(metadata.all_features);
metadata.default_target = table.get("default-target")
.and_then(|v| v.as_str()).map(|v| v.to_owned());
metadata.extra_targets = table.get("extra-targets").and_then(|f| f.as_array())
.and_then(|f| f.iter().map(|v| v.as_str().map(|v| v.to_owned())).collect());

This comment has been minimized.

Copy link
@pietroalbini

pietroalbini Mar 7, 2020

Member

You don't have to do it in this PR, but it would be nice to just use #[derive(Deserialize)].

@pietroalbini

This comment has been minimized.

Copy link
Member

pietroalbini commented Mar 7, 2020

Oh, also, at least for now we need to filter the targets to make sure someone doesn't use an unsupported target, and fail the build in that case.

@jyn514

This comment has been minimized.

Copy link
Member Author

jyn514 commented Mar 7, 2020

Oh, also, at least for now we need to filter the targets to make sure someone doesn't use an unsupported target, and fail the build in that case.

This is our current behavior for an unsupported default-target:

2020/03/07 12:51:25 [INFO] rustwide::cmd: [stderr] error[E0463]: can't find crate for `std`
2020/03/07 12:51:25 [INFO] rustwide::cmd: [stderr]   |
2020/03/07 12:51:25 [INFO] rustwide::cmd: [stderr]   = note: the `x86_64-pc-windows-gnu` target may not be installed
2020/03/07 12:51:25 [INFO] rustwide::cmd: [stderr] 
2020/03/07 12:51:25 [INFO] rustwide::cmd: [stderr] error: aborting due to previous error
2020/03/07 12:51:25 [INFO] rustwide::cmd: [stderr] 
2020/03/07 12:51:25 [INFO] rustwide::cmd: [stderr] For more information about this error, try `rustc --explain E0463`.
2020/03/07 12:51:25 [INFO] rustwide::cmd: [stderr] error: Could not document `hexponent`.

Our behavior for an unsupported extra-target is the same, I think it doesn't need to be handled explicitly.

@jyn514

This comment has been minimized.

Copy link
Member Author

jyn514 commented Mar 7, 2020

Addressed review comments.

@pietroalbini

This comment has been minimized.

Copy link
Member

pietroalbini commented Mar 7, 2020

Our behavior for an unsupported extra-target is the same, I think it doesn't need to be handled explicitly.

Keep in mind that we only show the build log of the default target, so users wouldn't be able to see those errors.

@jyn514

This comment has been minimized.

Copy link
Member Author

jyn514 commented Mar 7, 2020

Keep in mind that we only show the build log of the default target, so users wouldn't be able to see those errors.

Hmm, that does make it harder ... I don't want to fail the default build if one of the targets isn't installed, since that would require publishing a full point release. Maybe we could show a popup on the /crate page that says the target doesn't exist?

@jyn514

This comment has been minimized.

Copy link
Member Author

jyn514 commented Mar 7, 2020

Also I plan to make a follow-up PR very shortly which will install targets dynamically so I'm not sure it's worth worrying about.

jyn514 added 5 commits Mar 9, 2020
This is almost entirely untested.
@jyn514 jyn514 changed the title Allow crates to opt-in to building a single target [WIP] Allow crates to opt-in to building a single target Mar 9, 2020
@jyn514 jyn514 force-pushed the jyn514:opt-in-single-target branch from 802e4c0 to 591eaf2 Mar 9, 2020
@jyn514 jyn514 force-pushed the jyn514:opt-in-single-target branch from 591eaf2 to e06990b Mar 9, 2020
jyn514 added 2 commits Mar 9, 2020
@jyn514 jyn514 force-pushed the jyn514:opt-in-single-target branch from c89044e to ff96849 Mar 9, 2020
@jyn514

This comment has been minimized.

Copy link
Member Author

jyn514 commented Mar 10, 2020

This is ready for review.

@jyn514 jyn514 changed the title [WIP] Allow crates to opt-in to building a single target Allow crates to opt-in to building a single target Mar 10, 2020
src/docbuilder/metadata.rs Outdated Show resolved Hide resolved
src/docbuilder/metadata.rs Outdated Show resolved Hide resolved
@pietroalbini

This comment has been minimized.

Copy link
Member

pietroalbini commented Mar 11, 2020

Other than these two comments this looks good to be merged!

@jyn514

This comment has been minimized.

Copy link
Member Author

jyn514 commented Mar 11, 2020

Addressed review comments.

@jyn514 jyn514 merged commit e30c54b into rust-lang:master Mar 14, 2020
2 checks passed
2 checks passed
Test
Details
Docker
Details
@jyn514 jyn514 deleted the jyn514:opt-in-single-target branch Mar 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants
You can’t perform that action at this time.