Make tidy not traverse untracked directories by Milo123459 · Pull Request #112921 · rust-lang/rust

Milo123459 · 2023-06-22T11:13:55Z

Fixes #69291
(untested, still needs some work)

Not sure if this is the best way to do it either. Could anyone give some advice?

rustbot · 2023-06-22T11:14:01Z

r? @clubby789

(rustbot has picked a reviewer for you, use r? to override)

rust-log-analyzer · 2023-06-22T11:19:53Z

The job mingw-check-tidy failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

Getting action download info
Download action repository 'actions/checkout@v3' (SHA:c85c95e3d7251135ab7dc9ce3241c5835cc595a9)
Download action repository 'rust-lang/simpleinfra@master' (SHA:3999262f9f9933e66da0e5e168b90798c32a320e)
Download action repository 'actions/upload-artifact@v3' (SHA:0b7f8abb1508181956e8e162db84b466c27e18ce)
Complete job name: PR - mingw-check-tidy
git config --global core.autocrlf false
shell: /usr/bin/bash --noprofile --norc -e -o pipefail {0}
env:
  CI_JOB_NAME: mingw-check-tidy
---
Building wheels for collected packages: reuse
  Building wheel for reuse (pyproject.toml): started
  Building wheel for reuse (pyproject.toml): finished with status 'done'
  Created wheel for reuse: filename=reuse-1.1.0-cp310-cp310-manylinux_2_35_x86_64.whl size=180116 sha256=351235b2326fb4db7a18e257e13ce7896c5f77339521e2c2612e71e154800a19
  Stored in directory: /tmp/pip-ephem-wheel-cache-eio97r82/wheels/c2/3c/b9/1120c2ab4bd82694f7e6f0537dc5b9a085c13e2c69a8d0c76d
Installing collected packages: boolean-py, binaryornot, setuptools, reuse, python-debian, markupsafe, license-expression, jinja2, chardet
  Attempting uninstall: setuptools
    Found existing installation: setuptools 59.6.0
    Not uninstalling setuptools at /usr/lib/python3/dist-packages, outside environment /usr
---
Successfully tagged rust-ci:latest
Built container sha256:1f0fc30b39e0eb87b04c8f6a5ab4b9e4c68cacc46e75c03e94bb554317f9246c
Uploading finished image to https://ci-caches.rust-lang.org/docker/6c79211b759db8881e4e083f32c23f7a9fde027e88ac4e65b52f7f5e0405fbb6bd8d3cd81afe7617920fe598db5be42068b29a40aff8b5cde467364f4587bbd8

<botocore.awsrequest.AWSRequest object at 0x7f9afe4594d0>
gzip: stdout: Broken pipe
xargs: docker: terminated by signal 13
[CI_JOB_NAME=mingw-check-tidy]
[CI_JOB_NAME=mingw-check-tidy]
---
   Compiling tidy v0.1.0 (/checkout/src/tools/tidy)
error[E0308]: mismatched types
  --> src/tools/tidy/src/walk.rs:42:54
   |
42 |                 skip.remove(skip.iter().position(|x| **x = line))
   |                                                      ^^^^^^^^^^ expected `bool`, found `()`
error[E0308]: mismatched types
  --> src/tools/tidy/src/walk.rs:42:29
   |
   |
42 |                 skip.remove(skip.iter().position(|x| **x = line))
   |                      ------ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected `usize`, found `Option<usize>`
   |                      arguments to this method are incorrect
   |
   = note: expected type `usize`
              found enum `Option<usize>`
              found enum `Option<usize>`
note: method defined here
  --> /rustc/eff24c06d8f4397802b546aa2e52450e1022fc02/library/alloc/src/vec/mod.rs:1496:12

error[E0308]: mismatched types
  --> src/tools/tidy/src/walk.rs:42:17
   |
41 | /             if skip.contains(&line) {
42 | |                 skip.remove(skip.iter().position(|x| **x = line))
   | |                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected `()`, found `&str`
   | |_____________- expected this to be `()`
   |
help: consider using a semicolon here
   |
   |
42 |                 skip.remove(skip.iter().position(|x| **x = line));
help: consider using a semicolon here
   |
43 |             };
   |              +

clubby789 · 2023-06-22T23:12:59Z

I don't think we want to be running this command on every invocation of filter_dirs, since the output isn't going to change. Maybe this could be done in a OnceLock or similar

clubby789 · 2023-06-22T23:14:05Z

src/tools/tidy/src/walk.rs

+            String::from_utf8_lossy(&output.stdout).split('\n').into_iter().collect();
+        for line in stdout {
+            if skip.contains(&line) {
+                skip.remove(skip.iter().position(|x| **x = line))


Shouldn't this just add each line of stdout to skip?

The issue (unless I'm interpreting it wrong) says that you need to remove these elements from the skip array. But, that could be wrong.

The fix for this is probably to run git ls-files . --exclude-standard --others and skip files that it outputs in the default filter_dirs function

The existing skip array defines directories that we wish to skip. We want to extend this with any untracked directories - all directories in skip are tracked, so right now this wouldn't do anything even without the compile errors

clubby789 · 2023-06-22T23:14:55Z

src/tools/tidy/src/walk.rs

        "vendor",
    ];
+    let command =
+        Command::new("git").args(["ls-files", ".", "--exclude-standard", "--others"]).output();


This won't work properly if tidy is run from a subdirectory. Maybe this could be combined with git rev-parse --show-toplevel?

Also, ls-files will return each untracked file in a directory rather than the directory itself. I think --directory is the flag you want to prevent recursing into untracked dirs

This was the command that was posted in the linked issue, so I wasn't sure. I'll look into it.

clubby789 · 2023-06-22T23:17:12Z

src/tools/tidy/src/walk.rs

+        Command::new("git").args(["ls-files", ".", "--exclude-standard", "--others"]).output();
+    if let Ok(output) = command {
+        let stdout: Vec<&str> =
+            String::from_utf8_lossy(&output.stdout).split('\n').into_iter().collect();


nit: no need to collect when you iterate over it below

Milo123459 · 2023-06-23T07:01:58Z

I don't think we want to be running this command on every invocation, since the output isn't going to change. Maybe this could be done in a OnceLock or similar

I think in that case, if done in a OnceLock, would it not be beneficial to just put the entire array in a const or something?

clubby789 · 2023-06-23T10:02:34Z

Untracked directories can't be known at compile time, as they may change after tidy is compiled.

bors · 2023-07-05T19:31:06Z

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

Dylan-DPC · 2023-08-16T17:43:44Z

@Milo123459 any updates on this?

Milo123459 · 2023-08-16T19:32:45Z

dont have the time for this pr rn, sorry

this should theoretically work

f20c9a4

rustbot assigned clubby789 Jun 22, 2023

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 Jun 22, 2023

Milo123459 marked this pull request as draft June 22, 2023 16:05

clubby789 reviewed Jun 22, 2023

View reviewed changes

clubby789 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 26, 2023

Milo123459 closed this Aug 16, 2023

clubby789 mentioned this pull request Aug 16, 2023

tidy should not traverse untracked directories #69291

Open

Uh oh!

Conversation

Milo123459 commented Jun 22, 2023

Uh oh!

rustbot commented Jun 22, 2023

Uh oh!

rust-log-analyzer commented Jun 22, 2023

Uh oh!

clubby789 commented Jun 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

clubby789 Jun 22, 2023

Choose a reason for hiding this comment

Uh oh!

Milo123459 Jun 23, 2023

Choose a reason for hiding this comment

Uh oh!

clubby789 Jun 23, 2023

Choose a reason for hiding this comment

Uh oh!

clubby789 Jun 22, 2023

Choose a reason for hiding this comment

Uh oh!

clubby789 Jun 22, 2023

Choose a reason for hiding this comment

Uh oh!

Milo123459 Jun 23, 2023

Choose a reason for hiding this comment

Uh oh!

clubby789 Jun 22, 2023

Choose a reason for hiding this comment

Uh oh!

Milo123459 commented Jun 23, 2023

Uh oh!

clubby789 commented Jun 23, 2023

Uh oh!

bors commented Jul 5, 2023

Uh oh!

Dylan-DPC commented Aug 16, 2023

Uh oh!

Milo123459 commented Aug 16, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

clubby789 commented Jun 22, 2023 •

edited

Loading