Skip to content

set default for build.tidy-extra-checks to auto run all but shellcheck #144461

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

Closed

Conversation

lolbinarycat
Copy link
Contributor

@lolbinarycat lolbinarycat commented Jul 25, 2025

shellcheck is the only one of these tests which is not run in CI.

the auto: system seems to be working decently well, so I think it makes sense to enable it by default at some point.

unsure if we want to let this sit for a bit to make sure no issues come up, or optimize extra checks more, or if it's good to just move ahead with this. my only concern would be if it makes tidy extra slow on hdd systems.

r? @Kobzol

…t shellcheck

shellcheck is the only one of these tests which is not run in CI.
@rustbot rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc 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 Jul 25, 2025
@rustbot
Copy link
Collaborator

rustbot commented Jul 25, 2025

This PR modifies src/bootstrap/src/core/config.

If appropriate, please update CONFIG_CHANGE_HISTORY in src/bootstrap/src/utils/change_tracker.rs.

@Kobzol
Copy link
Member

Kobzol commented Jul 25, 2025

Can you measure how long it takes on your PC with/without the auto defaults? This should also be documented in the boostrap.example.toml file and have a change tracker entry.

@rust-log-analyzer
Copy link
Collaborator

The job aarch64-gnu-llvm-19-2 failed! Check out the build log: (web) (plain enhanced) (plain)

Click to see the possible cause of the failure (guessed by this bot)
tidy: Skipping binary file check, read-only filesystem
removing old virtual environment
creating virtual environment at '/checkout/obj/build/venv' using 'python3.12' and 'venv'
creating virtual environment at '/checkout/obj/build/venv' using 'python3.12' and 'virtualenv'
tidy error: virtualenv not found: you may need to install it:
`python3.12 -m pip install virtualenv`
If you see an error about "externally managed environment" when running the above command,
either install `virtualenv` using your system package manager
(e.g. `sudo apt-get install python3.12-virtualenv`) or create a virtual environment manually, install
`virtualenv` in it and then activate it before running tidy.

some tidy checks failed
Command has failed. Rerun with -v to see more details.
Build completed unsuccessfully in 0:00:45
  local time: Fri Jul 25 18:51:13 UTC 2025
  network time: Fri, 25 Jul 2025 18:51:13 GMT
##[error]Process completed with exit code 1.
Post job cleanup.

@lolbinarycat
Copy link
Contributor Author

Can you measure how long it takes on your PC with/without the auto defaults?

I believe it adds around 0.1 seconds, tho I have another PR i'm working on that lowers that down to 0.05 by eliminating the repeated calls to git.

This should also be documented in the boostrap.example.toml file and have a change tracker entry.

I was planning on adding a change tracker entry, but how should I document it in boostrap.example.toml? say it can be permanently disabled by setting it to the empty string? mention that it should be disabled on hdd systems with less memory?

@lolbinarycat
Copy link
Contributor Author

also, it looks like this should be disabled on CI unless explicitly enabled? otherwise every dist job will enable this, and not all of them have all the extra tools needed. other option is making all those jobs install those tools. also i think this would be run by distro maintainers where it previously was not.

perhaps a better idea would be enabling it by default only in the tools profile?

@Kobzol
Copy link
Member

Kobzol commented Jul 25, 2025

say it can be permanently disabled by setting it to the empty string?

Well we should definitely document the default there, and also the way how to disable it with the empty string, yes.

also, it looks like this should be disabled on CI unless explicitly enabled? otherwise every dist job will enable this, and not all of them have all the extra tools needed. other option is making all those jobs install those tools. also i think this would be run by distro maintainers where it previously was not.

Dist jobs shouldn't ever run tidy. That being said, the default should indeed be disabled on CI, because a lot of test jobs do run just x test and that does execute tidy by default.

Zalathar added a commit to Zalathar/rust that referenced this pull request Jul 29, 2025
…extra-checks-enable-for-tools, r=Kobzol

bootstrap: enable tidy auto extra checks on tools profile

alternative to rust-lang#144461

this won't affect CI or any `./configure` based workflows, and will also not affect every rust contributor like that PR will.  a slower rollout of this feature should reduce disruption if issues are discovered with it.

r? `@Kobzol`
Zalathar added a commit to Zalathar/rust that referenced this pull request Jul 29, 2025
…extra-checks-enable-for-tools, r=Kobzol

bootstrap: enable tidy auto extra checks on tools profile

alternative to rust-lang#144461

this won't affect CI or any `./configure` based workflows, and will also not affect every rust contributor like that PR will.  a slower rollout of this feature should reduce disruption if issues are discovered with it.

r? ``@Kobzol``
rust-timer added a commit that referenced this pull request Jul 29, 2025
Rollup merge of #144599 - lolbinarycat:bootstrap-build.tidy-extra-checks-enable-for-tools, r=Kobzol

bootstrap: enable tidy auto extra checks on tools profile

alternative to #144461

this won't affect CI or any `./configure` based workflows, and will also not affect every rust contributor like that PR will.  a slower rollout of this feature should reduce disruption if issues are discovered with it.

r? ``@Kobzol``
@bors
Copy link
Collaborator

bors commented Jul 30, 2025

☔ The latest upstream changes (presumably #144692) made this pull request unmergeable. Please resolve the merge conflicts.

@rustbot rustbot removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 31, 2025
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 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.

5 participants