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

.\x fmt std panics #107944

Closed
ChrisDenton opened this issue Feb 12, 2023 · 2 comments · Fixed by #107948
Closed

.\x fmt std panics #107944

ChrisDenton opened this issue Feb 12, 2023 · 2 comments · Fixed by #107948
Assignees
@jieyouxu
Labels
C-enhancement Category: An issue proposing an enhancement or a PR with one. E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)

Comments

@ChrisDenton
Copy link
Member

ChrisDenton commented Feb 12, 2023 •
edited by rustbot
Loading

Doing x test std will run the std's tests.

Doing x fmt std results in:

thread 'main' panicked at 'entry failed with std: The system cannot find the file specified. (os error 2)', format.rs:257:25
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

Doing x fmt library/std works but I expected fmt to work similarly to test.
@ChrisDenton ChrisDenton added the T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) label Feb 12, 2023
@jyn514
Copy link
Member

jyn514 commented Feb 12, 2023

Fixing this for std isn't hard, but fixing it in a general way is somewhat tricky because we don't have Steps to look at for format. I think we should only support shortcuts for directories, and only three directories deep to avoid making the search too expensive (I picked three so that tools in src/tools will be supported).

Relevant code:

rust/src/bootstrap/format.rs

Line 198 in d094016

walker.add(path);

@jyn514 jyn514 added C-enhancement Category: An issue proposing an enhancement or a PR with one. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. labels Feb 12, 2023
@jieyouxu
Copy link
Member

I would like to give this a try
@rustbot claim

@jieyouxu jieyouxu mentioned this issue Feb 12, 2023
Merged
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Feb 13, 2023
@matthiaskrgr
Rollup merge of rust-lang#107948 - jieyouxu:issue-107944, r=ozkanonur
7efb884 
Allow shortcuts to directories to be used for ./x.py fmt

Fixes rust-lang#107944.

Maximum recursive search depth is 3 and only accepts shortcuts for directories. If there are no shortcut candidates, the previous behavior to panic is preserved. If there are multiple candidates, the shortcut candidates are ignored.

After this change, `./x.py fmt std` no longer panics and formats `library/std` instead.
@bors bors closed this as completed in b10d744 Feb 14, 2023
nnethercote added a commit to nnethercote/rust that referenced this issue May 29, 2024
@nnethercote
Remove path choice from x fmt and add --all option.
5cf198d 
By default, `x fmt` formats/checks modified files. But it also lets you
choose one or more paths instead.

This adds significant complexity to `x fmt`. Explicit paths are
specified via `WalkBuilder::add` rather than `OverrideBuilder::add`. The
`ignore` library is not simple, and predicting the interactions between
the two mechanisms is difficult.

Here's a particularly interesting case.
- You can request a path P that is excluded by the `ignore` list in the
  `rustfmt.toml`. E.g. `x fmt tests/ui/` or `x fmt tests/ui/bitwise.rs`.
- `x fmt` will add P to the walker (via `WalkBuilder::add`), traverse it
  (paying no attention to the `ignore` list from the `rustfmt.toml`
  file, due to the different mechanism), and call `rustfmt` on every
  `.rs` file within it.
- `rustfmt` will do nothing to those `.rs` files, because it *also*
  reads `rustfmt.toml` and sees that they match the `ignore` list!

It took me *ages* to debug and understand this behaviour. Not good!

`x fmt` even lets you name a path below the current directory. This was
intended to let you do things like `x fmt std` that mirror things like
`x test std`. This works by looking for `std` and finding `library/std`,
and then formatting that. Unfortuantely, this motivating case now gives
an error. When support was added in rust-lang#107944, `library/std` was the only
directory named `std`. Since then, `tests/ui/std` was added, and so `x
fmt std` now gives an error.

In general, explicit paths don't seem particularly useful. The only two
cases `x fmt` really needs are:
- format/check the files I have modified (99% of uses)
- format/check all files

(While respecting the `ignore` list in `rustfmt.toml`, of course.)

So this commit moves to that model. `x fmt` will now give an error if
given an explicit path. `x fmt` now also supports a `--all` option. (And
running with `GITHUB_ACTIONS=true` also causes everything to be
formatted/checked, as before.) Much simpler!
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@jieyouxu jieyouxu

Labels
C-enhancement Category: An issue proposing an enhancement or a PR with one. E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Allow shortcuts to directories to be used for ./x.py fmt jieyouxu/rust
3 participants
@ChrisDenton @jyn514 @jieyouxu