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

Vendor libtest's dependencies in the rust-src component #78790

Merged
merged 1 commit into from
Nov 11, 2020

Conversation

Gankra
Copy link
Contributor

@Gankra Gankra commented Nov 5, 2020

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.

@rust-highfive
Copy link
Collaborator

r? @Mark-Simulacrum

(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
@alexcrichton
Copy link
Member

I'm actually somewhat surprised this works and it may be a bug in Cargo, because if lock files exist in locations other than the workspace root they should be ignored. In that sense it shouldn't matter where the lock file is for vendoring.

I also tried running this locally and it just vendored all of rustc's deps, so I'm curious how this is working locally for you now...

@Gankra
Copy link
Contributor Author

Gankra commented Nov 6, 2020

Hmm, maybe I'm triggering bugs in vendor on windows mingw?

Did you get more in your vendor than

addr2line
adler
cc
cfg-if
compiler_builtins
dlmalloc
fortanix-sgx-abi
getopts
gimli
hashbrown
hermit-abi
libc
miniz_oxide
object
rustc-demangle
unicode-width
wasi
(and the rustc-std-workspace crates)

@alexcrichton
Copy link
Member

Yeah when I run cargo vendor in library/test (with or without the top-level Cargo.lock in this dir) it vendors the entirety of the crates rust-lang/rust depends on (e.g. a hundred or so crates)

@Gankra
Copy link
Contributor Author

Gankra commented Nov 6, 2020

Note that this runs vendor inside the rust-src directory, so it shouldn't even be able to see rustc? And extra junk in the Cargo.lock doesn't affect the vendor, right? I believe that's why copying the lock is necessary too -- there isn't an actual workspace inside rust-src.

@alexcrichton
Copy link
Member

Oh dear I apologize, that I agree should work perfectly and makes sense as a result, ignore me!

@matklad
Copy link
Member

matklad commented Nov 6, 2020

Oh, this is perfect for IDEs, thanks! I wonder if we can leave Cargo.lock in the rust-src component as well? This will (maybe, I am not sure?) help intellij rust and rust-analyzer to just run cargo metadata inside of the rust-src component.

cc @vlad20012

@Gankra
Copy link
Contributor Author

Gankra commented Nov 6, 2020

Note that there is a Cargo.lock in the root of the rust-src component, but there's no workspace or .cargo/config connecting everything.

@matklad
Copy link
Member

matklad commented Nov 6, 2020

Riiight, I was thinking about Cargo.lock inside /library, so that we can look only at the library (and not on the rest of the compiller), but that's obviously useless without corrresponding Cargo.toml

Anyway, this is off-topic for this PR :)

@Gankra
Copy link
Contributor Author

Gankra commented Nov 6, 2020

@alexcrichton So who needs to sign off on this? Some team?

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.

I'm not sure about sign-offs, and I'm a little uncomfortable to r+ myself, but I'd say from the Cargo team perspective this looks good to me (and a rustbuild perspective), but @Mark-Simulacrum should probably sign-off too.

@Mark-Simulacrum
Copy link
Member

I'm not 100% sure we'll be able to write into the src directory for the lockfile -- IIRC, the dist builders have that marked as readonly. But I'm okay with checking in CI for that, since it might not be quite right. Ultimately if that doesn't work we can just copy the whole library directory or something along those lines.

@bors r+ rollup=iffy

@bors
Copy link
Contributor

bors commented Nov 6, 2020

📌 Commit dd68d0b has been approved by Mark-Simulacrum

@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 6, 2020
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Nov 8, 2020
…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.
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Nov 8, 2020
…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.
@Dylan-DPC-zz
Copy link

failed in rollup (#78885)

@bors r-

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Nov 9, 2020
@Gankra
Copy link
Contributor Author

Gankra commented Nov 9, 2020

hmm, I would have liked a chance to land on my own to be sure. As-is, I can't really make sense of why the specific breakage CI had
might happen.

@Mark-Simulacrum
Copy link
Member

Hm, let's actually try a @bors try -- I suspect the dist failure is not Windows specific, though I could of course be wrong :)

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Nov 17, 2020
* 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
* 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
@Keruspe
Copy link
Contributor

Keruspe commented Nov 19, 2020

This introduces a regression wrt distributions packaging. The dist step now requires network access, which it commonly cannot have during such a step of the packaging. Is there a way to make this cargo vendor call "offline"?

@Mark-Simulacrum
Copy link
Member

Please file a new issue -- I suspect the answer is yes, though it may require some work; presumably if you are running dist offline (which usually builds code implicitly you have already downloaded the relevant pieces here?), so it should be possible to avoid further network traffic.

@Gankra
Copy link
Contributor Author

Gankra commented Nov 19, 2020

vendor has an --offline flag for exactly this purpose.

@Keruspe
Copy link
Contributor

Keruspe commented Nov 19, 2020

Will open an issue after investigating, but I guess it might be a cargo vendor issue, not reading net.offline from .cargo/config

@Gankra
Copy link
Contributor Author

Gankra commented Nov 19, 2020

The implementation is a bit hacky, hiding the workspace and copying in the lockfile. So perhaps some other things need to be pulled in in a similar manner. That said, I would hope that --offline Just Works here.

ehuss added a commit to ehuss/rust that referenced this pull request Nov 30, 2020
bors bot added a commit to intellij-rust/intellij-rust that referenced this pull request Dec 7, 2020
6417: CARGO: fetch actual info about stdlb r=vlad20012 a=Undin

Previously, we always used hardcoded info about stdlib: its packages and their relationships.
But it prevented us from getting actual info about stdlib packages: dependencies, edition, features, etc.
As a result, some code in stdlib that depends on third-party crates (like `cfg-if`) cannot be properly taken into account while name resolution and type inference.

These changes introduce an option (as an experimental feature for now) to fetch actual info about stdlib packages.
Due to the fact that stdlib sources contain `Cargo.toml`, it's possible just to run `cargo metadata` to get all necessary info.
It allows us to get proper info about:
- edition. When stdlib started to use `2018` edition, plugin name resolution was broken in some places because of it. See #3835. We solved this issue by hardcoded edition for root stdlib packages depending on `rustc` version. Actual info about edition allows avoiding similar issues for future editions like edition 2021.
- dependencies. It fixes name resolution in cases when third-party libraries are used in stdlib packages. The most known case is the usage of `cfg-if` lib in `std::os` module that provides different API for different operating systems. Now name resolution and completion for child modules of `std::os` should work as expected.
- features. They also are taken into account while name resolution

![2020-11-20 13 41 58](https://user-images.githubusercontent.com/2539310/99798555-44615f00-2b42-11eb-8811-368c1a381ea6.gif)



Restrictions:
- works since 1.41.0
- the plugin calls `cargo vendor` to collect stdlib dependencies before rustc 1.49. Since rust-lang/rust#78790 `rust-src` component already contains `vendor` directory with the corresponding libraries

Known issues and future improvements:
- There isn't UI option to enable it. Currently, the feature can be enabled via `org.rust.cargo.fetch.actual.stdlib.metadata` experimental feature
- stdlib dependencies are shown as project dependencies. They should be part of `stdlib` node 
  <image src='https://user-images.githubusercontent.com/2539310/99792714-884f6680-2b38-11eb-94c6-e58112f45bf0.png' width='200px'/>
- Progress is not shown in `Sync` tool window
- Name resolution doesn't work properly for stdlib items inside stdlib dependencies because of [rustc-std-workspace-core](https://github.com/rust-lang/rust/tree/master/library/rustc-std-workspace-core) dependency and its friends. Not sure that we want to fix it


Closes #3864

Main step to fix the following issues: #3957 #4960 #5881 #6081
Part of #6104

changelog: TODO

6464: FIX: do not offer create method intention on foreign types r=vlad20012 a=Kobzol

The `CreateFunctionIntention` checks that the target module is from the user's workspace, but this check was missing for creating methods. This PR fixes that.

changelog: Create method intention is no longer offered for types from external or std crates.


Co-authored-by: Arseniy Pendryak <a.pendryak@yandex.ru>
Co-authored-by: Jakub Beránek <berykubik@gmail.com>
Mark-Simulacrum pushed a commit to Mark-Simulacrum/rust that referenced this pull request Dec 8, 2020
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 8, 2020
…ulacrum

[beta] backports

* [beta] always disable copy_file_range to avoid EOVERFLOW errors rust-lang#79008
* Enforce no-move rule of ReentrantMutex using Pin and fix UB in stdio rust-lang#77801
* bootstrap: use the same version number for rustc and cargo rust-lang#79133
* [beta] Revert "Enable ASLR for windows-gnu" rust-lang#79141
* [beta] revert rust-lang#78790, vendor libtest for rustc-src rust-lang#79571
* Mirror centos vault to S3 rust-lang#79435
*  [beta] Update cargo rust-lang#79739

This also bumps to non-dev stable compiler.

r? `@ghost`
ehuss added a commit to ehuss/rust that referenced this pull request Dec 16, 2020
…k-Simulacrum"

This reverts commit 7afc517, reversing
changes made to d4ea0b3.
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Dec 17, 2020
…k-Simulacrum

Revert rust-lang#78790 - rust-src vendoring

This reverts the rust-src vendor changes from rust-lang#78790. There were a few issues (see rust-lang#79218, rust-lang/cargo#8962, rust-lang/cargo#8963), that I don't think will get fixed in the next few days before the beta branch.

Fixes rust-lang#79218
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 17, 2020
Rollup of 11 pull requests

Successful merges:

 - rust-lang#79051 (Implement if-let match guards)
 - rust-lang#79877 (Allow `since="TBD"` for rustc_deprecated)
 - rust-lang#79882 (Fix issue rust-lang#78496)
 - rust-lang#80026 (expand-yaml-anchors: Make the output directory separator-insensitive)
 - rust-lang#80039 (Remove unused `TyEncoder::tcx` required method)
 - rust-lang#80069 (Test that `core::assert!` is valid)
 - rust-lang#80072 (Fixed conflict with drop elaboration and coverage)
 - rust-lang#80073 (Add support for target aliases)
 - rust-lang#80082 (Revert rust-lang#78790 - rust-src vendoring)
 - rust-lang#80097 (Add `popcount` and `popcnt` as doc aliases for `count_ones` methods.)
 - rust-lang#80103 (Remove docs for non-existent parameters in `rustc_expand`)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@eddyb
Copy link
Member

eddyb commented Mar 25, 2022

The implementation is a bit hacky, hiding the workspace and copying in the lockfile. So perhaps some other things need to be pulled in in a similar manner. That said, I would hope that --offline Just Works here.

Not sure where to post this but AFAICT this is fixable by generating a workspace Cargo.toml for the rust-src dist/vendor step, with a [patch.crates-io] section that contains:

rust/Cargo.toml

Lines 129 to 133 in 7941b3f

# See comments in `library/rustc-std-workspace-core/README.md` for what's going on
# here
rustc-std-workspace-core = { path = 'library/rustc-std-workspace-core' }
rustc-std-workspace-alloc = { path = 'library/rustc-std-workspace-alloc' }
rustc-std-workspace-std = { path = 'library/rustc-std-workspace-std' }

This could potentially even allow Cargo to stop hardcoding them - if we did ship that Cargo.toml, at least.

(cc @ehuss @Mark-Simulacrum)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet