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

Changing build.profiler forces an unnecessary rebuild of std #131812

Closed
Zalathar opened this issue Oct 17, 2024 · 3 comments · Fixed by #131816
Closed

Changing build.profiler forces an unnecessary rebuild of std #131812

Zalathar opened this issue Oct 17, 2024 · 3 comments · Fixed by #131816
Labels
A-code-coverage Area: Source-based code coverage (-Cinstrument-coverage) C-discussion Category: Discussion or questions that doesn't represent real issues. C-enhancement Category: An issue proposing an enhancement or a PR with one. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)

Comments

@Zalathar
Copy link
Contributor

In theory, opening config.toml and toggling build.profiler between false and true should be cheap. Setting it to true should cause the profiler runtime to be built, without rebuilding std. And setting it to false should simply cause the rest of the build to pretend that any previously-built compiler runtime doesn't exist.

In practice, toggling build.profiler does trigger a rebuild of std, which also naturally triggers a rebuild of the compiler.

I think this is an artifact of library/profiler_runtime being considered an optional dependency of std. And I think that is just an artifact of how the profiler runtime was originally added (#42433), not something that inherently has to be true.

Of course, the tricky part will be fixing this without accidentally breaking anything.

@Zalathar Zalathar added A-code-coverage Area: Source-based code coverage (-Cinstrument-coverage) C-enhancement Category: An issue proposing an enhancement or a PR with one. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Oct 17, 2024
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Oct 17, 2024
@jieyouxu jieyouxu added E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. E-needs-investigation Call for partcipation: This issues needs some investigation to determine current status E-hard Call for participation: Hard difficulty. Experience needed to fix: A lot. and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. labels Oct 17, 2024
@Zalathar
Copy link
Contributor Author

Zalathar commented Oct 17, 2024

I think bootstrap itself is behaving correctly here, in that if the profiler runtime really were a dependency of std, then enabling it should trigger a std rebuild.

So it's more of a questionable configuration, in that it really shouldn't be a dependency.

Also, toggling the profiler off/on is presumably not very common, especially now that the profiler can be built without the src/llvm-project submodule (#129295). So this should be a pretty low priority relative to its difficulty.

@onur-ozkan
Copy link
Member

Additional note: "profiler" feature can still be enabled on std through this option:

#std-features = ["panic_unwind"]
even if we remove it from the build.profiler logic.

@onur-ozkan onur-ozkan added C-discussion Category: Discussion or questions that doesn't represent real issues. and removed E-needs-investigation Call for partcipation: This issues needs some investigation to determine current status E-hard Call for participation: Hard difficulty. Experience needed to fix: A lot. labels Oct 17, 2024
@Zalathar
Copy link
Contributor Author

I looked into this briefly, and the fix turns out to have been simpler than I was expecting (fingers crossed), so I've filed #131816.

bors added a commit to rust-lang-ci/rust that referenced this issue Oct 17, 2024
Make `profiler_builtins` an optional dependency of sysroot, not std

This avoids unnecessary rebuilds of std (and the compiler) when `build.profiler` is toggled off or on.

Fixes rust-lang#131812.

---

Background: The `profiler_builtins` crate has been an optional dependency of std (behind a cargo feature) ever since it was added back in rust-lang#42433. But as far as I can tell that has only ever been a convenient way to force the crate to be built, not a genuine dependency.

The side-effect of this false dependency is that toggling `build.profiler` causes a rebuild of std and the compiler, which shouldn't be necessary. This PR therefore makes `profiler_builtins` an optional dependency of the dummy sysroot crate (rust-lang#108865), rather than a dependency of std.

What makes this change so small is that all of the necessary infrastructure already exists. Previously, bootstrap would enable the `profiler` feature on the sysroot crate, which would forward that feature to std. Now, enabling that feature directly enables sysroot's `profiler_builtins` dependency instead.

---

I believe this is more of a bootstrap change than a libs change, so tentatively:
r? bootstrap
@bors bors closed this as completed in c926476 Oct 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-code-coverage Area: Source-based code coverage (-Cinstrument-coverage) C-discussion Category: Discussion or questions that doesn't represent real issues. C-enhancement Category: An issue proposing an enhancement or a PR with one. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants