opened
Keruspe
username_0
Sadly, we need to copy the [patch] section from the toplevel Cargo.toml
for the --offline mode to properly work

Fixes #79218

Pros: it seems to work!
Cons: There's now this warning during the build: `warning: patch for the non root package will be ignored, specify patch at the workspace root:`
rustbuild: don't require network for vendoring libtest
issue
created
rust-highfive
username_1
r? @m-ou-se

(rust_highfive has picked a reviewer for you, use r? to override)
comment
created
jyn514
username_2
r? @username_3 maybe
cc @username_5
comment
created
Mark-Simulacrum
username_3
I think we should try to avoid that warning from Cargo. I am unsure if we can afford to edit test/Cargo.toml during the build (some build systems mount the source directory as read only IIRC). I guess there's not much we can do other than that, right?

If it's not really going to be easy to avoid, though, I think we can live with the warning -- I don't think there's any other easy way to remove it. Obviously we're doing something not entirely compatible with Cargo's expectations of how the workspace looks :)

cc @rust-lang/cargo -- a case where Cargo's unconditional warnings are painful
comment
created
Keruspe
username_0
Is there something more I can do here, or maybe another solution I could try?
comment
created
ehuss
username_4
I'd like to propose that #78790 be reverted on beta, just to give some more time to figure out what to do.  The original motivation for #78790 was for `build-std` which is a nightly-only feature, so I don't think there is a need to ship it on beta (there are other users interested in it, but they can wait).

I personally would prefer to avoid duplicating the patch table, and avoid the incessant warnings. Unfortunately I can't think of a trivial alternative.  My best suggestion would be to use `cargo metadata` to fetch the dependencies of `test`, and then use the vendor directory from `PlainSourceTarball` and selectively copy out the minimum set of dependencies needed for libtest.  It will be tricky, since there are a few edge cases, but I think it can be done in under 100 lines of code. 

Here's a gist of a prototype: https://gist.github.com/username_4/943de115b8fe535090145b1cbfc51542.  The code is rough, and needs error handling and doesn't actually do any copying.  It will also be a bit tricky to share the vendor directory between `PlainSourceTarball` and `Src`.  

It might be good to consider using [`cargo_metadata`](https://crates.io/crates/cargo_metadata) in bootstrap to make the code a little shorter, which would allow removing [these definitions](https://github.com/rust-lang/rust/blob/cf9bfdb8726c07974c8db93f70ee7213ee20c563/src/bootstrap/metadata.rs#L10-L28), but I understand if there is reluctance to add another dependency to bootstrap.

There is one minor oddity with how `cargo metadata` works, that would cause this to pull in the `jobserver` dependency which the current `cargo vendor` solution doesn't do, but I think that's fine.  (It has to do with how features are resolved with `cargo metadata` in a workspace.)
comment
created
Mark-Simulacrum
username_3
I am personally fine reverting it on beta - I think @username_5 said they'd need a workaround anyway since Firefox(?) needs to build on older versions of Rust tarballs, so one or two more releases is no big deal.

@username_0 do you think you'd be up for implementing what @username_4 suggests? Otherwise we can try to find someone else (I might be able to find time).
comment
created
Keruspe
username_0
@username_3 Not sure I'll be able to find enough time to work on this during december
comment
created
Gankra
username_5
Different thing -- we need this to build our tsan CI, which is pinned to a specific nightly. So this will only be an issue when we need to bump that (probably won't need to for a while, so it's fine).
comment
created
ehuss
username_4
I posted a beta revert at #79571.
comment
created
Mark-Simulacrum
username_3
I don't think I'll have time this week, and perhaps not next, to sit down and try to work through the proposed solution -- quite busy right now. That said the beta revert should land soon (#79838) and that'll give us another cycle here, putting us well into next year, which should hopefully help...
comment
created
bors
username_6
:umbrella: The latest upstream changes (presumably #80105) made this pull request unmergeable. Please [resolve the merge conflicts](https://rustc-dev-guide.rust-lang.org/git.html#conflicts).

*Note that reviewers usually do not review pull requests until merge conflicts are resolved!* Once you resolve the conflicts, you should change the labels applied by username_6 to indicate that your PR is ready for review. Post this as a comment to change the labels:
```
@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author
```
comment
created
jyn514
username_2
@username_0 FYI just because https://github.com/rust-lang/rust/pull/79571 landed doesn't mean this needs to be closed - I think @username_5 would still like to add rust-src to `x.py dist` at some point, at which point we'll need a fix for the networking issue.
comment
created
Gankra
username_5
Yeah firefox's tsan builds rely on vendored libtest. We're pinned to the nightlies where it worked, so we have a fair bit of time, but I'd like to see this reimplemented properly.
comment