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

rustbuild: don't require network for vendoring libtest #79276

Closed
wants to merge 1 commit into from

Conversation

Keruspe
Copy link
Contributor

@Keruspe Keruspe commented Nov 21, 2020

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:

@rust-highfive
Copy link
Collaborator

r? @m-ou-se

(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 21, 2020
@jyn514 jyn514 added the T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) label Nov 21, 2020
@jyn514
Copy link
Member

jyn514 commented Nov 21, 2020

r? @Mark-Simulacrum maybe
cc @Gankra

@Mark-Simulacrum
Copy link
Member

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

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

Fixes rust-lang#79218

Signed-off-by: Marc-Antoine Perennou <Marc-Antoine@Perennou.com>
@Mark-Simulacrum Mark-Simulacrum 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-review Status: Awaiting review from the assignee but also interested parties. labels Nov 22, 2020
@Keruspe
Copy link
Contributor Author

Keruspe commented Nov 25, 2020

Is there something more I can do here, or maybe another solution I could try?

@ehuss
Copy link
Contributor

ehuss commented Nov 30, 2020

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/ehuss/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 in bootstrap to make the code a little shorter, which would allow removing these definitions, 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.)

@Mark-Simulacrum Mark-Simulacrum added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 30, 2020
@Mark-Simulacrum
Copy link
Member

I am personally fine reverting it on beta - I think @Gankra 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.

@Keruspe do you think you'd be up for implementing what @ehuss suggests? Otherwise we can try to find someone else (I might be able to find time).

@Keruspe
Copy link
Contributor Author

Keruspe commented Nov 30, 2020

@Mark-Simulacrum Not sure I'll be able to find enough time to work on this during december

@Gankra
Copy link
Contributor

Gankra commented Nov 30, 2020

I am personally fine reverting it on beta - I think @Gankra 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.

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).

@ehuss
Copy link
Contributor

ehuss commented Nov 30, 2020

I posted a beta revert at #79571.

@Mark-Simulacrum
Copy link
Member

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...

@bors
Copy link
Contributor

bors commented Dec 17, 2020

☔ The latest upstream changes (presumably #80105) made this pull request unmergeable. Please resolve the merge 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 bors 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

@Keruspe Keruspe closed this Dec 17, 2020
@jyn514
Copy link
Member

jyn514 commented Dec 18, 2020

@Keruspe FYI just because #79571 landed doesn't mean this needs to be closed - I think @Gankra 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.

@Gankra
Copy link
Contributor

Gankra commented Dec 18, 2020

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

x.py dist shouldn’t require network access
8 participants