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.

Sign up for GitHub

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

Jump to bottom

Add a tidy rule forbidding modules named build #133404

Closed
wants to merge 5 commits into from
Closed

Add a tidy rule forbidding modules named build #133404

wants to merge 5 commits into from

Conversation

Zalathar
Copy link
Contributor

The module directory compiler/rustc_mir_build/src/build/ causes a number of contributor papercuts:

  • GitHub's go-to-file feature doesn't work for any files in this directory.
  • We need a negated rule in .gitignore to override the blanket ignore rule for build/.
  • Some git-adjacent tools automatically parse .gitignore but don't understand negative rules, so they inappropriately ignore the directory.
  • Some tidy checks are accidentally not applied within this directory, probably for similar reasons.

Any one of these might be fixable or ignorable, but all of them together suggest that allowing modules named build is just not worth the hassle, especially when it isn't even necessary.

This PR therefore adds a tidy check forbidding mod build; and similar, and renames the existing modules that would be forbidden by that check. There are no functional changes in the affected modules.

(The check also has the side-effect of forbidding build.rs files that aren't build scripts, which seems net-positive.)

@Zalathar
Remove a stray backtick
11fe251 
This was not detected by `x test tidy`, presumably because it's inside a
directory named "build". Fix it now, because later changes in this PR will
cause it to be detected.
@Zalathar
Rename rustc_mir_build::build to builder
a6444ca
@Zalathar
Rename rustc_expand::build to builder
7ee19f8
@Zalathar
Rename rustc_next_trait_solver::solve::inspect::build to builder
ccc06c0
@rustbot
Copy link
Collaborator

rustbot commented Nov 24, 2024

r? @pnkfelix

rustbot has assigned @pnkfelix.
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 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) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver) labels Nov 24, 2024
@Zalathar Zalathar force-pushed the builder branch 2 times, most recently from e814133 to b9d5728 Compare November 24, 2024 03:25
@Zalathar
Add a tidy check prohibiting mod build
508e78f 
Because we have a .gitignore rule for `build/`, having a module directory named
"build" causes various problems for tools that try to skip ignored files.

Forbidding this in tidy is trickier than it looks, because tidy itself tries to
skip ignored files. So the rule instead looks in non-ignored files for module
declarations that would imply the existence of potentially-ignored module
directories.

This also has the side-effect of prohibiting `build.rs` files that aren't build
scripts.
@Zalathar
Copy link
Contributor Author

Replaced manual LazyLock with static_regex! (diff).

@jieyouxu
Copy link
Member

r? jieyouxu

@rustbot rustbot assigned jieyouxu and unassigned pnkfelix Nov 24, 2024
@Zalathar
Copy link
Contributor Author

Ignoring non-root build directories was done deliberately in #106440.

@jieyouxu
Copy link
Member

jieyouxu commented Nov 24, 2024 •
edited
Loading

Hm, I'm inclined to revert it back to /build/, because /build will match against any subdirectories name build/, including e.g. compiler/rustc_mir_build/src/build/foo.rs which I find very footgunny. @pnkfelix wanted to be able to run ./x.py from subdirectories (which would create build/ underneath subdirectories, but creating build/ in subdirectories seem very strange).

Anyway, waiting for team input in https://rust-lang.zulipchat.com/#narrow/channel/326414-t-infra.2Fbootstrap/topic/The.20.60build.2F.60.20ignore.20rule.

@rustbot label -S-waiting-on-review +S-waiting-on-team

@rustbot rustbot added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 24, 2024
@bors
Copy link
Contributor

bors commented Nov 24, 2024

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

@jieyouxu
Copy link
Member

Hi @Zalathar, thanks for the PR. I'm going to close this in favor of #133592, as I think not having the special .gitignore hacks is overall a better experience and it avoids having to add more special cases to tidy.

I would be in favor, however, of renaming rustc_mir_build/src/build/ to something... more descriptive if those more familiar with MIR has better names...

@jieyouxu jieyouxu closed this Dec 15, 2024
@Zalathar Zalathar deleted the builder branch December 16, 2024 01:41
@Zalathar Zalathar mentioned this pull request Dec 16, 2024
Merged
@Zalathar
Copy link
Contributor Author

Renaming rustc_mir_build::build extracted to #134365.

jhpratt added a commit to jhpratt/rust that referenced this pull request Dec 17, 2024
@jhpratt
Rollup merge of rust-lang#134365 - Zalathar:builder, r=nnethercote
cc5392a 
Rename `rustc_mir_build::build` to `builder`

GitHub's “Go to file” feature silently ignores all files in this module, presumably because they are in a directory named `build`, which is mistaken for a build-output directory. That makes it meaningfully harder to view those files and their history via GitHub.

This PR sidesteps that issue by renaming `build` to `builder`, which in context has basically the same meaning, but can't be mistaken for a build-output directory.

---

Extracted from rust-lang#133404, after rust-lang#133592 changed the .gitignore rule from `build/` to `/build`. The problem of GitHub ignoring these files still exists even after that change, which suggests that GitHub's behaviour is a hard-coded heuristic that isn't influenced by the repository's git settings.

Currently this PR doesn't include the tidy rule forbidding `build` as a module name, but that could be added if people think it's a good idea.
jhpratt added a commit to jhpratt/rust that referenced this pull request Dec 17, 2024
@jhpratt
Rollup merge of rust-lang#134365 - Zalathar:builder, r=nnethercote
7e72c3e 
Rename `rustc_mir_build::build` to `builder`

GitHub's “Go to file” feature silently ignores all files in this module, presumably because they are in a directory named `build`, which is mistaken for a build-output directory. That makes it meaningfully harder to view those files and their history via GitHub.

This PR sidesteps that issue by renaming `build` to `builder`, which in context has basically the same meaning, but can't be mistaken for a build-output directory.

---

Extracted from rust-lang#133404, after rust-lang#133592 changed the .gitignore rule from `build/` to `/build`. The problem of GitHub ignoring these files still exists even after that change, which suggests that GitHub's behaviour is a hard-coded heuristic that isn't influenced by the repository's git settings.

Currently this PR doesn't include the tidy rule forbidding `build` as a module name, but that could be added if people think it's a good idea.
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Dec 17, 2024
@rust-timer
Unrolled build for rust-lang#134365
eb8cab0 
Rollup merge of rust-lang#134365 - Zalathar:builder, r=nnethercote

Rename `rustc_mir_build::build` to `builder`

GitHub's “Go to file” feature silently ignores all files in this module, presumably because they are in a directory named `build`, which is mistaken for a build-output directory. That makes it meaningfully harder to view those files and their history via GitHub.

This PR sidesteps that issue by renaming `build` to `builder`, which in context has basically the same meaning, but can't be mistaken for a build-output directory.

---

Extracted from rust-lang#133404, after rust-lang#133592 changed the .gitignore rule from `build/` to `/build`. The problem of GitHub ignoring these files still exists even after that change, which suggests that GitHub's behaviour is a hard-coded heuristic that isn't influenced by the repository's git settings.

Currently this PR doesn't include the tidy rule forbidding `build` as a module name, but that could be added if people think it's a good idea.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@jieyouxu jieyouxu

Labels
A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). 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. WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Zalathar @rustbot @jieyouxu @bors @pnkfelix