Skip to content

run spellcheck as a tidy extra check in ci #145025

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

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

lolbinarycat
Copy link
Contributor

This is probably how it should've been done from the start.

r? @Kobzol

@rustbot rustbot added A-CI Area: Our Github Actions CI 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-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. T-rustdoc-frontend Relevant to the rustdoc-frontend team, which will review and decide on the web UI/UX output. WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver) labels Aug 6, 2025
@rustbot
Copy link
Collaborator

rustbot commented Aug 6, 2025

Some changes occurred in HTML/CSS/JS.

cc @GuillaumeGomez, @jsha, @lolbinarycat

@rust-log-analyzer

This comment has been minimized.

@Kobzol
Copy link
Member

Kobzol commented Aug 7, 2025

I would suggest preinstalling the typos binary in the tidy Dockerfile (by downloading a pinned version from GH releases), and then also adding some download functionality from GitHub releases for local execution (or just use cargo install probably easier), same as how tidy installs Python/JS deps for the user.

@rustbot rustbot added A-tidy Area: The tidy tool T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Aug 8, 2025
@rust-log-analyzer

This comment has been minimized.

@klensy
Copy link
Contributor

klensy commented Aug 8, 2025

Can you keep previous logic: check version first and only if not found/wrong build it? I have typos in PATH downloaded from github and not build via cargo, so current logic will build second one for me.

P.S. in case of building typos, it will be nice to drop note in log like building typos or smth.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@@ -66,3 +67,65 @@ pub fn parse_gitmodules(target_dir: &Path) -> Vec<String> {

submodules_paths
}

/// If the given executible is installed with the given version, use that,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// If the given executible is installed with the given version, use that,
/// If the given executable is installed with the given version, use that,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you would really think that enabling typo checking would catch that ^^;

@Kobzol
Copy link
Member

Kobzol commented Aug 9, 2025

I think that tidy is being executed from some other directory. You should specify the working directory when running the typos binary to be the source root which is passed to tidy.

Copy link
Member

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

The comment on top of SPELLCHECK_DIRS is outdated, could you remove it please?

The build of typos seems to take ~1 minute on CI. We could try to build it with --debug, or preinstall it in the Dockerfile, but in any case 1 minute isn't so terrible.


/// If the given executible is installed with the given version, use that,
/// otherwise install via cargo.
pub fn ensure_version_or_cargo_install(
Copy link
Member

Choose a reason for hiding this comment

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

Let's move this to tidy. It's only used in one place now, no need to increase bootstrap's (and other tools') build time by putting it into build_helper.

@lolbinarycat
Copy link
Contributor Author

lolbinarycat commented Aug 9, 2025

The build of typos seems to take ~1 minute on CI. We could try to build it with --debug, or preinstall it in the Dockerfile, but in any case 1 minute isn't so terrible.

this seems like the exact usecase for opt-level 1 or 2, where you care about minimizing run + build time. i'll do local testing to see what works best.

UPDATE: local testing seems very noisy, opt level 0 was fastest, but then weirdly 3 was next fastest, with 1 and 2 being almost twice as slow as the others. this seems strange, as the non-rebuild runtime of tidy seemed about the same no matter the opt level. I guess we can just stick -Copt-level=0 in RUSTFLAGS, since it doesn't seem like typos relies much on optimizations being enabled. preinstalling in the Dockerfile also seems like a good idea, but I don't know enough about docker or github releases to do that properly (we do need to make sure the versions stay in sync, tho, probably with a .version file)

@rust-log-analyzer

This comment has been minimized.

@rustbot rustbot added the A-rustdoc-search Area: Rustdoc's search feature label Aug 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-CI Area: Our Github Actions CI A-rustdoc-search Area: Rustdoc's search feature A-testsuite Area: The testsuite used to check the correctness of rustc A-tidy Area: The tidy tool 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) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. T-rustdoc-frontend Relevant to the rustdoc-frontend team, which will review and decide on the web UI/UX output. WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants