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

feat: new arg --cargo-config for passing --config to cargo #1913

Merged
merged 2 commits into from
May 29, 2024

Conversation

weihanglo
Copy link
Member

@weihanglo weihanglo commented May 29, 2024

This is another step toward making opt-dist work in sandboxed environments.

--cargo-config is added to the following subcommands:

  • binary_stats compile
  • bench_runtime_local
  • bench_local
  • profile_local

Rationale

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

We can leverage cargo --config to specify additional Cargo's configuration.
Training data then could build with correct vendored settings via the --cargo-config flag.

See also


r? @Kobzol

(Sorry I haven't figure out how to add tests for this)

@weihanglo
Copy link
Member Author

To integrate this with opt-dist, we need the following patch:

diff --git a/src/tools/opt-dist/src/training.rs b/src/tools/opt-dist/src/training.rs
index 67c850b196d..9e417b01573 100644
--- a/src/tools/opt-dist/src/training.rs
+++ b/src/tools/opt-dist/src/training.rs
@@ -16,7 +16,7 @@ fn init_compiler_benchmarks(
     // Run rustc-perf benchmarks
     // Benchmark using profile_local with eprintln, which essentially just means
     // don't actually benchmark -- just make sure we run rustc a bunch of times.
-    cmd(&[
+    let mut cmd = cmd(&[
         env.cargo_stage_0().as_str(),
         "run",
         "-p",
@@ -41,7 +41,16 @@ fn init_compiler_benchmarks(
     .env("RUST_LOG", "collector=debug")
     .env("RUSTC", env.rustc_stage_0().as_str())
     .env("RUSTC_BOOTSTRAP", "1")
-    .workdir(&env.rustc_perf_dir())
+    .workdir(&env.rustc_perf_dir());
+
+    // Respect `.cargo/config.toml` in the rustc source. This is useful when the
+    // source is from a tarball and contains vendored source settings.
+    let dot_cargo_config_toml  = env.checkout_path().join(".cargo").join("config.toml");
+    if dot_cargo_config_toml.is_file() {
+        cmd.arg("--cargo-config").arg(dot_cargo_config_toml);
+    }
+
+    cmd
 }
 
 /// Describes which `llvm-profdata` binary should be used for merging PGO profiles.

This approach has a strong assumption: if there is a .cargo/config.toml in the root of the rustc source, it will be applied unconditionally.

We could consider other alternatives, such as providing a --cargo-config flag in opt-dist, allowing downstream packagers to use it as needed.

Copy link
Contributor

@Kobzol Kobzol left a comment

Choose a reason for hiding this comment

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

Left a few nits, otherwise LGTM.

Could you please also document this in collector/README.md? Thanks!

collector/src/bin/collector.rs Outdated Show resolved Hide resolved
collector/src/toolchain.rs Outdated Show resolved Hide resolved
collector/src/toolchain.rs Outdated Show resolved Hide resolved
This is another step toward making opt-dist work in sandboxed environments.

`--cargo-config` is added to the following subcommands:

* `binary_stats compile`
* `bench_runtime_local`
* `bench_local`
* `profile_local`

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

We can leverage [`cargo --config`][1] to specify additional Cargo's
configuration. Training data then could build with correct vendored
settings via the `--cargo-config` flag.

[1]: https://doc.rust-lang.org/nightly/cargo/commands/cargo.html#option-cargo---config
@weihanglo
Copy link
Member Author

Everything has been pluralized (except the CLI argument name) and simplified. Thank you for your prompt review!

@weihanglo weihanglo requested a review from Kobzol May 29, 2024 08:52
Copy link
Contributor

@Kobzol Kobzol left a comment

Choose a reason for hiding this comment

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

Thanks! We'll need to update the rustc-perf version in rust-lang/rust after this.

@Kobzol Kobzol merged commit dd649a7 into rust-lang:master May 29, 2024
11 checks passed
@weihanglo weihanglo deleted the cargo-config branch May 29, 2024 09:00
@weihanglo
Copy link
Member Author

Thanks! We'll need to update the rustc-perf version in rust-lang/rust after this.

Will do that. Maybe after rust-lang/measureme#229?

@Kobzol
Copy link
Contributor

Kobzol commented May 29, 2024

Sure, there are probably more changes in general that need to be made before the builds can be truly self-contained (licensing etc., we'll need to update the rust_team_data dependency).

bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 3, 2024
…fig, r=<try>

feat(opt-dist): respect existing .cargo/config.toml

This should be the last piece toward self-contained `opt-dist` (I believe).

It fixes the issue described in rust-lang#125465

> * 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]
> [^1]: https://github.com/rust-lang/rustc-perf/blob/4f313add609f43e928e98132358e8426ed3969ae/collector/src/compile/benchmark/mod.rs#L164-L173

See also

* <rust-lang/rustc-perf#1913>
* <rust-lang#125465>
* https://rust-lang.zulipchat.com/#narrow/stream/247081-t-compiler.2Fperformance/topic/tempfile.20in.20rustc-perf.20make.20it.20hard.20to.20configure.20vendor

r​? Kobzol
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 3, 2024
…fig, r=<try>

feat(opt-dist): respect existing .cargo/config.toml

This should be the last piece toward self-contained `opt-dist` (I believe).

It fixes the issue described in rust-lang#125465

> * 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]
> [^1]: https://github.com/rust-lang/rustc-perf/blob/4f313add609f43e928e98132358e8426ed3969ae/collector/src/compile/benchmark/mod.rs#L164-L173

See also

* <rust-lang/rustc-perf#1913>
* <rust-lang#125465>
* https://rust-lang.zulipchat.com/#narrow/stream/247081-t-compiler.2Fperformance/topic/tempfile.20in.20rustc-perf.20make.20it.20hard.20to.20configure.20vendor

r​? Kobzol
compiler-errors added a commit to compiler-errors/rust that referenced this pull request Jun 4, 2024
…onfig, r=Kobzol

feat(opt-dist): new flag `--benchmark-cargo-config`

This should be the last piece toward self-contained `opt-dist` (I believe).

The flag propagates cargo configs to `rustc-perf --cargo-config`,
which is particularly useful when the environment is air-gapped,
and you want to use the default set of training crates vendored
in the rustc-src tarball.

It fixes the issue described in rust-lang#125465

> * 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]
> [^1]: https://github.com/rust-lang/rustc-perf/blob/4f313add609f43e928e98132358e8426ed3969ae/collector/src/compile/benchmark/mod.rs#L164-L173

See also

* <rust-lang/rustc-perf#1913>
* <rust-lang#125465>
* https://rust-lang.zulipchat.com/#narrow/stream/247081-t-compiler.2Fperformance/topic/tempfile.20in.20rustc-perf.20make.20it.20hard.20to.20configure.20vendor

r​? Kobzol
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Jun 4, 2024
…onfig, r=Kobzol

feat(opt-dist): new flag `--benchmark-cargo-config`

This should be the last piece toward self-contained `opt-dist` (I believe).

The flag propagates cargo configs to `rustc-perf --cargo-config`,
which is particularly useful when the environment is air-gapped,
and you want to use the default set of training crates vendored
in the rustc-src tarball.

It fixes the issue described in rust-lang#125465

> * 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]
> [^1]: https://github.com/rust-lang/rustc-perf/blob/4f313add609f43e928e98132358e8426ed3969ae/collector/src/compile/benchmark/mod.rs#L164-L173

See also

* <rust-lang/rustc-perf#1913>
* <rust-lang#125465>
* https://rust-lang.zulipchat.com/#narrow/stream/247081-t-compiler.2Fperformance/topic/tempfile.20in.20rustc-perf.20make.20it.20hard.20to.20configure.20vendor

r​? Kobzol
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jun 4, 2024
Rollup merge of rust-lang#125930 - weihanglo:opt-dist-respect-cargo-config, r=Kobzol

feat(opt-dist): new flag `--benchmark-cargo-config`

This should be the last piece toward self-contained `opt-dist` (I believe).

The flag propagates cargo configs to `rustc-perf --cargo-config`,
which is particularly useful when the environment is air-gapped,
and you want to use the default set of training crates vendored
in the rustc-src tarball.

It fixes the issue described in rust-lang#125465

> * 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]
> [^1]: https://github.com/rust-lang/rustc-perf/blob/4f313add609f43e928e98132358e8426ed3969ae/collector/src/compile/benchmark/mod.rs#L164-L173

See also

* <rust-lang/rustc-perf#1913>
* <rust-lang#125465>
* https://rust-lang.zulipchat.com/#narrow/stream/247081-t-compiler.2Fperformance/topic/tempfile.20in.20rustc-perf.20make.20it.20hard.20to.20configure.20vendor

r​? Kobzol
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants