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.

Sign up for GitHub

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

Jump to bottom

Check if rust-src contains a vendor dir, and patch it in #8834

Merged
merged 2 commits into from
Nov 12, 2020
Merged

Check if rust-src contains a vendor dir, and patch it in #8834

merged 2 commits into from
Nov 12, 2020

Conversation

Gankra
Copy link
Contributor

@Gankra Gankra commented Nov 5, 2020 •
edited
Loading

This is the cargo side of rust-lang/wg-cargo-std-aware#23

Note that this design naively assumes there is only one version of each package. It does not robustly verify this, and will presumably just cryptically fail to resolve dependencies.

See rust-lang/rust#78790 for the other half of this change.

@rust-highfive
Copy link

r? @Eh2406

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 5, 2020
@Gankra Gankra mentioned this pull request Nov 5, 2020
Merged
@Gankra
Copy link
Contributor Author

Gankra commented Nov 5, 2020 •
edited
Loading

Note that these two PRs do not need to be synchronized at all.

  • The old cargo has no idea the vendor dir exists and won't notice it.
  • The new cargo will gracefully fall back to the current behaviour if a vendor dir isn't found.

This adds ~10MB to the uncompressed rust-src component.

alexcrichton
alexcrichton reviewed Nov 6, 2020
View reviewed changes
Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks pretty good to me! I think it may be difficult to add an automated test for this at this time until it's upstream and published, but can you confirm that this works locally?

src/cargo/core/compiler/standard_lib.rs Outdated Show resolved Hide resolved
@Gankra
Copy link
Contributor Author

Gankra commented Nov 6, 2020

Yeah I verified this locally by:

  • building my new version of x.py dist
  • copying the rust-src vendor dir it produced to my rustup nightly toolchain
  • running -Zbuild-std on a local vendored project

I'm pretty confident it works because this method caught several mistakes I made while writing this! (like needing to copy the lockfile for vendor after all).

@alexcrichton
Copy link
Member

Ok great! I'm realizing now though that we can probably test this locally since we have basically a mock of the rust-std component and I think it should be possible to add a dependency only present in the vendor directory?

Also could you add some tests where test has its own dependency (like getopts) but then std doesn't use it? (to make sure no warnings are generated)

@Gankra
Copy link
Contributor Author

Gankra commented Nov 6, 2020

Can you point me to a test I can use as a template for this kind of thing?

@alexcrichton
Copy link
Member

I believe all those tests are housed here

@Gankra
Copy link
Contributor Author

Gankra commented Nov 6, 2020

@alexcrichton it occurs to me that I can tweak the vendored version to not have any hardcoded rustc-std-workspace crates and instead derive everything from the vendor. In theory this would let rust add new rustc-std-workspace crates without changing cargo. Is that desirable?

@alexcrichton
Copy link
Member

That'd be awesome yeah! Less hardcoded stuff in Cargo always sounds good to me

@Gankra
Copy link
Contributor Author

Gankra commented Nov 6, 2020

(test being a member would still be hardcoded)

@Gankra
Copy link
Contributor Author

Gankra commented Nov 6, 2020

New version that derives the existence of the std packages from the vendor pushed up.

@Gankra Gankra force-pushed the rust-src-vendor branch 2 times, most recently from 6a96a14 to ec620e2 Compare November 6, 2020 18:23
@Gankra
Copy link
Contributor Author

Gankra commented Nov 6, 2020

Here's my first crack at testing the vendor logic.

alexcrichton
alexcrichton reviewed Nov 6, 2020
View reviewed changes
Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One test I think that'd be good to add is to publish a version of the crates.io crate which would break the compilation if used (e.g. invalid syntax), and that way we can be sure Cargo doesn't touch it.

tests/testsuite/mock-std/vendor/registry-dep-only-used-by-test/Cargo.toml
[package]
name = "registry-dep-only-used-by-test"
version = "1.0.0"
authors = ["Alex Crichton <[email protected]>"]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well why thank you!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy pasted these xp

tests/testsuite/standard_lib.rs Show resolved Hide resolved
@Gankra
Copy link
Contributor Author

Gankra commented Nov 6, 2020

One test I think that'd be good to add is to publish a version of the crates.io crate which would break the compilation if used.

This is a good idea, I've added it to the default environment every test uses to ensure all tests incidentally check this.

@Gankra
Copy link
Contributor Author

Gankra commented Nov 6, 2020

I'm a bit confused by the CI failure that happened on ubuntu-latest, nightly, i686-unknown-linux-gnu. It doesn't show up locally (even with deny-warnings). I don't have a linux box setup to test it either. Any ideas?

@alexcrichton
Copy link
Member

Are you sure you're testing with a nightly toolchain? Those tests only run on nightly and they're failing locally for me on mac with cargo +nightly test

@Gankra
Copy link
Contributor Author

Gankra commented Nov 6, 2020

I'll go dig out my macbook and see it it repro's there.

@Gankra
Copy link
Contributor Author

Gankra commented Nov 6, 2020

Ah, cut one too many corners and forgot that git won't add empty directories by default. Worked locally because the dirs still existed.

@alexcrichton
Copy link
Member

Looks great to me, thanks! @ehuss did you want to take a look or shall I r+?

@Gankra
Copy link
Contributor Author

Gankra commented Nov 7, 2020

Squashed

Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Nov 8, 2020
@Dylan-DPC
Rollup merge of rust-lang#78790 - Gankra:rust-src-vendor, r=Mark-Simu…
c713b73 
…lacrum

Vendor libtest's dependencies in the rust-src component

This is the Rust side of rust-lang/wg-cargo-std-aware#23

Note that this won't produce a useful result for `cargo -Zbuild-std` if there are multiple versions of a crate vendored, but will otherwise produce a valid vendor dir.

See rust-lang/cargo#8834 for the other half of this change.
@Gankra
Copy link
Contributor Author

Gankra commented Nov 10, 2020

Ok, removed the fallback mode

@Gankra
Copy link
Contributor Author

Gankra commented Nov 10, 2020

(this change of course means build_std won't compile until the other rust changes make it into nightly)

bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 11, 2020
@bors
Auto merge of rust-lang#78790 - Gankra:rust-src-vendor, r=Mark-Simula…
7afc517 
…crum

Vendor libtest's dependencies in the rust-src component

This is the Rust side of rust-lang/wg-cargo-std-aware#23

Note that this won't produce a useful result for `cargo -Zbuild-std` if there are multiple versions of a crate vendored, but will otherwise produce a valid vendor dir.

See rust-lang/cargo#8834 for the other half of this change.
@ehuss
Copy link
Contributor

ehuss commented Nov 12, 2020

I pushed a small change to add some error context.

@bors r+

@bors
Copy link
Contributor

bors commented Nov 12, 2020

📌 Commit 85b5b18 has been approved by ehuss

@bors 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 Nov 12, 2020
@bors
Copy link
Contributor

bors commented Nov 12, 2020

⌛ Testing commit 85b5b18 with merge 8662ab4...

@bors
Copy link
Contributor

bors commented Nov 12, 2020

☀️ Test successful - checks-actions
Approved by: ehuss
Pushing 8662ab4 to master...

@bors bors merged commit 8662ab4 into rust-lang:master Nov 12, 2020
@ehuss ehuss mentioned this pull request Nov 12, 2020
Closed
m-ou-se added a commit to m-ou-se/rust that referenced this pull request Nov 12, 2020
@m-ou-se
Rollup merge of rust-lang#78972 - ehuss:update-cargo, r=ehuss
50c1b1e 
Update cargo

5 commits in d5556aeb8405b1fe696adb6e297ad7a1f2989b62..8662ab427a8d6ad8047811cc4d78dbd20dd07699
2020-11-04 22:20:36 +0000 to 2020-11-12 03:47:53 +0000
- Check if rust-src contains a vendor dir, and patch it in (rust-lang/cargo#8834)
- Improve performance of almost fresh builds (rust-lang/cargo#8837)
- Use u32/64::to/from_le_bytes instead of bit fiddling (rust-lang/cargo#8847)
- Avoid constructing an anyhow::Error when not necessary (rust-lang/cargo#8844)
- Skip extracting .cargo-ok files from packages (rust-lang/cargo#8835)
m-ou-se added a commit to m-ou-se/rust that referenced this pull request Nov 12, 2020
@m-ou-se
Rollup merge of rust-lang#78972 - ehuss:update-cargo, r=ehuss
ef77a43 
Update cargo

5 commits in d5556aeb8405b1fe696adb6e297ad7a1f2989b62..8662ab427a8d6ad8047811cc4d78dbd20dd07699
2020-11-04 22:20:36 +0000 to 2020-11-12 03:47:53 +0000
- Check if rust-src contains a vendor dir, and patch it in (rust-lang/cargo#8834)
- Improve performance of almost fresh builds (rust-lang/cargo#8837)
- Use u32/64::to/from_le_bytes instead of bit fiddling (rust-lang/cargo#8847)
- Avoid constructing an anyhow::Error when not necessary (rust-lang/cargo#8844)
- Skip extracting .cargo-ok files from packages (rust-lang/cargo#8835)
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Nov 17, 2020
@Gankra
Bug 1671691 - Enable Rust stdlib instrumentation. r=decoder
9a0f581 
* Bumps the tsan toolchain to rust-nightly-2020-11-14 that has my patches to make -Zbuild-std work in vendored environments:
  * rust-lang/cargo#8834
  * rust-lang/rust#78790

* Passes -Zbuild-std to cargo when MOZ_TSAN is defined (mk_add_options --enable-thread-sanitizer)

* Removes generic Rust supressions and adds much more specific ones
    * One presumed upstream false positive from tsan not understanding the code
    * One actual upstream bug tsan found (yay!)
    * One new real issue uncovered
    * One issue that probably already existed intermittently but I happened to hit

Differential Revision: https://phabricator.services.mozilla.com/D97165
sidvishnoi pushed a commit to sidvishnoi/gecko-webmonetization that referenced this pull request Nov 18, 2020
@Gankra
Bug 1671691 - Enable Rust stdlib instrumentation. r=decoder
28415b9 
* Bumps the tsan toolchain to rust-nightly-2020-11-14 that has my patches to make -Zbuild-std work in vendored environments:
  * rust-lang/cargo#8834
  * rust-lang/rust#78790

* Passes -Zbuild-std to cargo when MOZ_TSAN is defined (mk_add_options --enable-thread-sanitizer)

* Removes generic Rust supressions and adds much more specific ones
    * One presumed upstream false positive from tsan not understanding the code
    * One actual upstream bug tsan found (yay!)
    * One new real issue uncovered
    * One issue that probably already existed intermittently but I happened to hit

Differential Revision: https://phabricator.services.mozilla.com/D97165
This was referenced Dec 9, 2020
Closed
Closed
@phil-opp phil-opp mentioned this pull request Dec 10, 2020
Closed
@alexcrichton alexcrichton mentioned this pull request Dec 11, 2020
Merged
bors added a commit that referenced this pull request Dec 12, 2020
@bors
Auto merge of #8968 - alexcrichton:revert-8834, r=ehuss
c68c622 
Revert recent build-std vendoring changes

This reverts #8834 to address #8962 and #8963

cc `@Gankra`
@ehuss ehuss mentioned this pull request Dec 18, 2020
Open
@ehuss ehuss added this to the 1.49.0 milestone Feb 6, 2022
@glandium
Copy link
Contributor

For the record, as we've been using this patch for sanitizer builds for Firefox, from the original comment: "Note that this design naively assumes there is only one version of each package.", well, this now doesn't work anymore because libstd directly and indirectly depends on two versions of cfg-if.

@glandium glandium mentioned this pull request Feb 1, 2024
Closed
@glandium glandium mentioned this pull request Jun 12, 2024
Closed
@glandium glandium mentioned this pull request Aug 5, 2024
Merged
@glandium glandium mentioned this pull request Sep 3, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matklad matklad matklad left review comments

@alexcrichton alexcrichton alexcrichton left review comments

@ehuss ehuss ehuss left review comments

Assignees

@Eh2406 Eh2406

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
1.49.0
Development

Successfully merging this pull request may close these issues.
8 participants
@Gankra @rust-highfive @alexcrichton @ehuss @bors @glandium @matklad @Eh2406