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

bootstrap: vendor crates required by opt-dist to collect profiles #125465

Merged
merged 3 commits into from
Jun 9, 2024

Conversation

weihanglo
Copy link
Member

@weihanglo weihanglo commented May 23, 2024

These are the default package set required by opt-dist to correctly work,
hence for people wanting to build a production grade of rustc in a
sandboxed / air-gapped environment, these need to be vendored.

The size of rustc-src-nightly.tar.xz before and after this change:

  • Before: 298M
  • After: 323M (+8%)

Size change might or might not be a concern.
See the previous discussion: #125166 (comment)

Previous efforts on making:


Note that extra works still need to be done to make it fully vendored.

  • The current pinned rustc-perf uses tempfile::Tempdir as the working
    directory when collecting profiles from some of these packages.
    This "tmp" working directory usage make it impossible for Cargo to pick
    up the correct vendor sources setting in .cargo/config.toml bundled
    in the rustc-src tarball. 1
  • opt-dist verifies the final built rustc against a subset of rustc test
    suite. However it rolls out its own config.toml without setting
    vendor = true, and that results in ./vendor/ directory removed.
    2

Footnotes

  1. https://github.com/rust-lang/rustc-perf/blob/4f313add609f43e928e98132358e8426ed3969ae/collector/src/compile/benchmark/mod.rs#L164-L173

  2. https://github.com/rust-lang/rust/blob/606afbb617a2949a4e35c4b0258ff94c980b9451/src/tools/opt-dist/src/tests.rs#L62-L77

@rustbot
Copy link
Collaborator

rustbot commented May 23, 2024

r? @albertlarsan68

rustbot has assigned @albertlarsan68.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added 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) labels May 23, 2024
@weihanglo
Copy link
Member Author

r? @Mark-Simulacrum as you reviewed the previous two PRs.
cc @Kobzol as you have been working hard in this area :)

@Kobzol
Copy link
Contributor

Kobzol commented May 24, 2024

r? @Kobzol

This makes sense, and it is nicer than packaging everything from rustc-perf, which is quite large.

@bors r+ rollup

@bors
Copy link
Contributor

bors commented May 24, 2024

📌 Commit 51a87fc has been approved by Kobzol

It is now in the queue for this repository.

@rustbot rustbot assigned Kobzol and unassigned Mark-Simulacrum May 24, 2024
@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 May 24, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request May 24, 2024
bootstrap: vendor crates required by opt-dist to collect profiles

These are the default package set required by opt-dist to correctly work,
hence for people wanting to build a production grade of rustc in a
sandboxed / air-gapped environment, these need to be vendored.

The size of `rustc-src-nightly.tar.xz` before and after this change:

* Before: 298M
* After: 323M (+8%)

Size change might or might not be a concern.
See the previous discussion: rust-lang#125166 (comment)

Previous efforts on making:

* rust-lang#125125
* rust-lang#125166

---

Note that extra works still need to be done to make it fully vendored.

* The current pinned rustc-perf uses `tempfile::Tempdir` as the working
  directory when collecting profiles from some of these packages.
  This "tmp" working directory usage make it impossible for Cargo to pick
  up the correct vendor sources setting in `.cargo/config.toml` bundled
  in the rustc-src tarball. [^1]
* opt-dist verifies the final built rustc against a subset of rustc test
  suite. However it rolls out its own `config.toml` without setting
  `vendor = true`, and that results in `./vendor/` directory removed.
  [^2]

[^1]: https://github.com/rust-lang/rustc-perf/blob/4f313add609f43e928e98132358e8426ed3969ae/collector/src/compile/benchmark/mod.rs#L164-L173
[^2]: https://github.com/rust-lang/rust/blob/606afbb617a2949a4e35c4b0258ff94c980b9451/src/tools/opt-dist/src/tests.rs#L62-L77
bors added a commit to rust-lang-ci/rust that referenced this pull request May 24, 2024
…ml, r=<try>

fix(opt-dist): respect existing config.toml

This is another step toward making opt-dist work in sandboxed environments. See also <rust-lang#125465>.

opt-dist verifies the final built rustc against a subset of rustc test
suite. However it overwrote the pre-existing `config.toml` [^1],
and that results in ./vendor/ directory removed [^2].

Instead of overwriting, this patch use `--set <config-value>` to
override paths to rustc / cargo / llvm-config.

[^1]: https://github.com/rust-lang/rust/blob/606afbb617a2949a4e35c4b0258ff94c980b9451/src/tools/opt-dist/src/tests.rs#L62-L77
[^2]: https://github.com/rust-lang/rust/blob/8679004993f08807289911d9f400f4ac4391d2bc/src/bootstrap/bootstrap.py#L1057
bors added a commit to rust-lang-ci/rust that referenced this pull request May 24, 2024
…iaskrgr

Rollup of 6 pull requests

Successful merges:

 - rust-lang#125455 (Make `clamp` inline)
 - rust-lang#125465 (bootstrap: vendor crates required by opt-dist to collect profiles )
 - rust-lang#125477 (Run rustfmt on files that need it.)
 - rust-lang#125481 (Fix the dead link in the bootstrap README)
 - rust-lang#125482 (Notify kobzol after changes to `opt-dist`)
 - rust-lang#125489 (Revert problematic opaque type change)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request May 24, 2024
…iaskrgr

Rollup of 6 pull requests

Successful merges:

 - rust-lang#125455 (Make `clamp` inline)
 - rust-lang#125465 (bootstrap: vendor crates required by opt-dist to collect profiles )
 - rust-lang#125477 (Run rustfmt on files that need it.)
 - rust-lang#125481 (Fix the dead link in the bootstrap README)
 - rust-lang#125482 (Notify kobzol after changes to `opt-dist`)
 - rust-lang#125489 (Revert problematic opaque type change)

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

@bors r-
I guess this failed here #125491 (comment)

@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 May 24, 2024
@Kobzol
Copy link
Contributor

Kobzol commented May 24, 2024

That seems unlikely, that failure looks like a spurious network error. It happened even before bootstrap was invoked, and in a builder that doesn't even do dist.

@Kobzol
Copy link
Contributor

Kobzol commented May 24, 2024

@bors r+

@bors
Copy link
Contributor

bors commented May 24, 2024

📌 Commit 51a87fc has been approved by Kobzol

It is now in the queue for this repository.

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 24, 2024
@bjorn3
Copy link
Member

bjorn3 commented May 24, 2024

Maybe add them to tidy's license check too? Without this PR they can't be added as the license check needs deps to be either vendored or internet access to be allowed and internet access is not allowed in all contexts where tidy runs. And it makes sense to me to run the license checker on everything we ship in either binary or source form to avoid surprises.

@Kobzol
Copy link
Contributor

Kobzol commented May 24, 2024

Does tidy check the licenses of all the dependencies and crates that we vendor? E.g. all dependencies of cargo etc.?

@weihanglo
Copy link
Member Author

For cargo part, yes. I don't know about other things.

@Kobzol
Copy link
Contributor

Kobzol commented Jun 5, 2024

I'm not sure about the licensing part, but the rest looks good to me. Have you tried to do a vendored build of opt-dist, to see if it works end-to-end? We probably won't have any testing for this.

(Big thanks for pushing all these changes through, btw! It's great to provide the optimized workflow for more people that build their own compiler).

@weihanglo
Copy link
Member Author

weihanglo commented Jun 7, 2024

@Kobzol yes. I ran src/ci/docker/run.sh dist-x86_64-linux with some modifications and it works. No crate was downloaded during the build. Verified by checking the existence of $CARGO_HOME/registry,git}. Without --benchmark-cargo-config provided crates will be downloaded.

Click to see the diff (basically call opt-dist local with linux-ci args)

diff --git a/src/ci/docker/host-x86_64/dist-x86_64-linux/Dockerfile b/src/ci/docker/host-x86_64/dist-x86_64-linux/Dockerfile
index 4e1aae98221..f1911fdea96 100644
--- a/src/ci/docker/host-x86_64/dist-x86_64-linux/Dockerfile
+++ b/src/ci/docker/host-x86_64/dist-x86_64-linux/Dockerfile
@@ -66,6 +66,7 @@ ENV PGO_HOST=x86_64-unknown-linux-gnu
 ENV HOSTS=x86_64-unknown-linux-gnu
 
 ENV RUST_CONFIGURE_ARGS \
+      --enable-vendor \
       --enable-full-tools \
       --enable-sanitizers \
       --enable-profiler \
@@ -80,10 +81,22 @@ ENV RUST_CONFIGURE_ARGS \
       --set rust.lto=thin \
       --set rust.codegen-units=1
 
-ENV SCRIPT python3 ../x.py build --set rust.debug=true opt-dist && \
-    ./build/$HOSTS/stage0-tools-bin/opt-dist linux-ci -- python3 ../x.py dist \
+ENV SCRIPT mkdir -p /tmp/tmp-multistage/ && \
+  ln -s /checkout/.cargo /tmp/tmp-multistage/.cargo && \
+  ln -s /checkout/vendor /tmp/tmp-multistage/vendor && \
+  python3 ../x.py build --set rust.debug=true opt-dist && \
+    ./build/$HOSTS/stage0-tools-bin/opt-dist local \
+    --target-triple x86_64-unknown-linux-gnu \
+    --checkout-dir /checkout \
+    --artifact-dir /tmp/tmp-multistage/opt-artifacts \
+    --llvm-dir /rustroot \
+    --llvm-shared \
+    --benchmark-cargo-config /checkout/.cargo/config.toml \
+    --use-bolt \
+    --skipped-tests tests/ui/process/nofile-limit.rs \
+    -- python3 ../x.py dist \
     --host $HOSTS --target $HOSTS \
     --include-default-paths \
     build-manifest bootstrap
 ENV CARGO_TARGET_X86_64_UNKNOWN_LINUX_GNU_LINKER=clang
 
diff --git a/src/tools/opt-dist/src/main.rs b/src/tools/opt-dist/src/main.rs
index e4271a6e2dd..c141ff72c9a 100644
--- a/src/tools/opt-dist/src/main.rs
+++ b/src/tools/opt-dist/src/main.rs
@@ -132,7 +132,7 @@ fn create_environment(args: Args) -> anyhow::Result<(Environment, Vec<String>)>
                 .checkout_dir(checkout_dir.clone())
                 .host_llvm_dir(llvm_dir)
                 .artifact_dir(artifact_dir)
-                .build_dir(checkout_dir)
+                .build_dir(checkout_dir.join("obj"))
                 .prebuilt_rustc_perf(rustc_perf_checkout_dir)
                 .shared_llvm(llvm_shared)
                 .use_bolt(use_bolt)

@Kobzol
Copy link
Contributor

Kobzol commented Jun 7, 2024

Awesome :)

@weihanglo
Copy link
Member Author

rust-lang/team and rust-lang/measureme have no license. I guess it's fine just adding one for them?

Yeah, we should propagate upstream and get approvals from relevant contributors@m for each of those repos (shouldn't be many people).

@Mark-Simulacrum I think the license and git source issues have been resolved. Would you mind reviewing this? We're vendoring rustc-perf in 1.80.0, so it might better for this to go hand-in-hand with that before new nightly branches off.

@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 Jun 7, 2024
@Mark-Simulacrum Mark-Simulacrum self-assigned this Jun 7, 2024
@Mark-Simulacrum
Copy link
Member

Nightly probably already branched, but I'll take a look on the weekend and maybe nominate for beta backport depending on details.

src/tools/build_helper/src/metrics.rs Outdated Show resolved Hide resolved
src/tools/tidy/src/deps.rs Outdated Show resolved Hide resolved
@Mark-Simulacrum
Copy link
Member

r=me with nits fixed

maybe nominate for beta backport depending on details.

I think given the changes here I'm not currently inclined to backport it.

These are the default package set required by opt-dist to correctly work,
hence for people wanting to build a production grade of rustc in a
sandboxed / air-gapped environment, these need to be vendored.

The size of `rustc-src-nightly.tar.xz` before and after this change:

* Before: 298M
* After: 323M (+8%)

These crates are the default set of packages required by opt-dist
to correctly work, hence for people wanting to build a production grade
of rustc in an sandboxed / air-gapped environment, these need to be vendored.

The size of `rustc-src-nightly.tar.xz` before and after this change:

* Before: 298M
* After: 323M (+8%)

Size change might or might not be a concern.
See the previous discussion: rust-lang#125166 (comment)

Previous efforts on making:

* rust-lang#125125
* rust-lang#125166

---

Note that extra works still need to be done to make it fully vendored.

* The current pinned rustc-perf uses `tempfile::Tempdir` as the working
  directory when collecting profiles from some of these packages.
  This "tmp" working directory usage make it impossible for Cargo to pick
  up the correct vendor sources setting in `.cargo/config.toml` bundled
  in the rustc-src tarball. [^1]
* opt-dist verifies the final built rustc against a subset of rustc test
  suite. However it rolls out its own `config.toml` without setting
  `vendor = true`, and that results in `./vendor/` directory removed.
  [^2]

[^1]: https://github.com/rust-lang/rustc-perf/blob/4f313add609f43e928e98132358e8426ed3969ae/collector/src/compile/benchmark/mod.rs#L164-L173
[^2]: https://github.com/rust-lang/rust/blob/606afbb617a2949a4e35c4b0258ff94c980b9451/src/tools/opt-dist/src/tests.rs#L62-L77
`ring` is included because it is an optional dependency of `hyper`,
which is a training data in rustc-pref for optimized build.
The license of it is generally `ISC AND MIT AND OpenSSL`,
though the `package.license` field is not set.

See briansmith/ring#902

`git+https://github.com/rust-lang/team` git source is from
`rust_team_data`, which is used by `site` in src/tools/rustc-perf.
This doesn't really a part of the compiler,
so doesn't really affect the bootstrapping process.

See rust-lang/rustc-perf#1914.
@weihanglo
Copy link
Member Author

nits are fixed, and the CI is all green now. Thanks for the review people.

@bors r=Mark-Simulacrum

@bors
Copy link
Contributor

bors commented Jun 9, 2024

📌 Commit 3778703 has been approved by Mark-Simulacrum

It is now in the queue for this repository.

@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 Jun 9, 2024
jieyouxu added a commit to jieyouxu/rust that referenced this pull request Jun 9, 2024
…Simulacrum

bootstrap: vendor crates required by opt-dist to collect profiles

These are the default package set required by opt-dist to correctly work,
hence for people wanting to build a production grade of rustc in a
sandboxed / air-gapped environment, these need to be vendored.

The size of `rustc-src-nightly.tar.xz` before and after this change:

* Before: 298M
* After: 323M (+8%)

Size change might or might not be a concern.
See the previous discussion: rust-lang#125166 (comment)

Previous efforts on making:

* rust-lang#125125
* rust-lang#125166

---

Note that extra works still need to be done to make it fully vendored.

* The current pinned rustc-perf uses `tempfile::Tempdir` as the working
  directory when collecting profiles from some of these packages.
  This "tmp" working directory usage make it impossible for Cargo to pick
  up the correct vendor sources setting in `.cargo/config.toml` bundled
  in the rustc-src tarball. [^1]
* opt-dist verifies the final built rustc against a subset of rustc test
  suite. However it rolls out its own `config.toml` without setting
  `vendor = true`, and that results in `./vendor/` directory removed.
  [^2]

[^1]: https://github.com/rust-lang/rustc-perf/blob/4f313add609f43e928e98132358e8426ed3969ae/collector/src/compile/benchmark/mod.rs#L164-L173
[^2]: https://github.com/rust-lang/rust/blob/606afbb617a2949a4e35c4b0258ff94c980b9451/src/tools/opt-dist/src/tests.rs#L62-L77
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 9, 2024
Rollup of 6 pull requests

Successful merges:

 - rust-lang#125041 (Enable GVN for `AggregateKind::RawPtr`)
 - rust-lang#125253 (Add `FRAC_1_SQRT_2PI` constant to f16/f32/f64/f128)
 - rust-lang#125465 (bootstrap: vendor crates required by opt-dist to collect profiles )
 - rust-lang#125470 (Add release notes for 1.79.0)
 - rust-lang#125963 (Remove hard-coded hashes from codegen tests)
 - rust-lang#126188 (Fix documentation for `impl_common_helpers` in `run-make-support`)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 4ca52f1 into rust-lang:master Jun 9, 2024
6 checks passed
@rustbot rustbot added this to the 1.81.0 milestone Jun 9, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jun 9, 2024
Rollup merge of rust-lang#125465 - weihanglo:opt-dist-vendor, r=Mark-Simulacrum

bootstrap: vendor crates required by opt-dist to collect profiles

These are the default package set required by opt-dist to correctly work,
hence for people wanting to build a production grade of rustc in a
sandboxed / air-gapped environment, these need to be vendored.

The size of `rustc-src-nightly.tar.xz` before and after this change:

* Before: 298M
* After: 323M (+8%)

Size change might or might not be a concern.
See the previous discussion: rust-lang#125166 (comment)

Previous efforts on making:

* rust-lang#125125
* rust-lang#125166

---

Note that extra works still need to be done to make it fully vendored.

* The current pinned rustc-perf uses `tempfile::Tempdir` as the working
  directory when collecting profiles from some of these packages.
  This "tmp" working directory usage make it impossible for Cargo to pick
  up the correct vendor sources setting in `.cargo/config.toml` bundled
  in the rustc-src tarball. [^1]
* opt-dist verifies the final built rustc against a subset of rustc test
  suite. However it rolls out its own `config.toml` without setting
  `vendor = true`, and that results in `./vendor/` directory removed.
  [^2]

[^1]: https://github.com/rust-lang/rustc-perf/blob/4f313add609f43e928e98132358e8426ed3969ae/collector/src/compile/benchmark/mod.rs#L164-L173
[^2]: https://github.com/rust-lang/rust/blob/606afbb617a2949a4e35c4b0258ff94c980b9451/src/tools/opt-dist/src/tests.rs#L62-L77
@weihanglo weihanglo deleted the opt-dist-vendor branch June 9, 2024 21:16
@michaelwoerister
Copy link
Member

🎉

bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 10, 2024
…r-ozkan

Update `rustc-perf` submodule before running tidy

Since rust-lang#125465, `tidy` checks `src/tools/rustc-perf`, so we need to have it checked out before running `tidy`.

Fixes: rust-lang#126224

r? `@onur-ozkan`
lcnr pushed a commit to lcnr/rust that referenced this pull request Jun 12, 2024
This is needed for fixing the missing license issue.

See rust-lang#125465.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. 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.