Skip to content

Tidy Python improvements #135950

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

Merged
merged 4 commits into from
Jan 24, 2025
Merged

Tidy Python improvements #135950

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5122c06
Refactor Python linting and formatting in tidy
Kobzol Jan 23, 2025
cd7be92
Document Python formatting and linting in the rustc-dev-guide
Kobzol Jan 23, 2025
9a85104
Make virtualenv creation in tidy more robust
Kobzol Jan 23, 2025
0f334ec
Update Python 3 versions in tidy
Kobzol Jan 23, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 25 additions & 3 deletions src/doc/rustc-dev-guide/src/conventions.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
This file offers some tips on the coding conventions for rustc. This
This file offers some tips on the coding conventions for rustc. This
chapter covers [formatting](#formatting), [coding for correctness](#cc),
[using crates from crates.io](#cio), and some tips on
[structuring your PR for easy review](#er).
Expand All @@ -25,6 +25,7 @@ pass the <!-- date-check: nov 2022 --> `--edition=2021` argument yourself when c
`rustfmt` directly.

[fmt]: https://github.com/rust-dev-tools/fmt-rfcs

[`rustfmt`]:https://github.com/rust-lang/rustfmt

## Formatting C++ code
Expand All @@ -40,6 +41,26 @@ When modifying that code, use this command to format it:
This uses a pinned version of `clang-format`, to avoid relying on the local
environment.

## Formatting and linting Python code

The Rust repository contains quite a lof of Python code. We try to keep
it both linted and formatted by the [ruff][ruff] tool.

When modifying Python code, use this command to format it:
```sh
./x test tidy --extra-checks=py:fmt --bless
```

and the following command to run lints:
```sh
./x test tidy --extra-checks=py:lint
```

This uses a pinned version of `ruff`, to avoid relying on the local
environment.

[ruff]: https://github.com/astral-sh/ruff

<a id="copyright"></a>

<!-- REUSE-IgnoreStart -->
Expand Down Expand Up @@ -84,7 +105,7 @@ Using `_` in a match is convenient, but it means that when new
variants are added to the enum, they may not get handled correctly.
Ask yourself: if a new variant were added to this enum, what's the
chance that it would want to use the `_` code, versus having some
other treatment? Unless the answer is "low", then prefer an
other treatment? Unless the answer is "low", then prefer an
exhaustive match. (The same advice applies to `if let` and `while
let`, which are effectively tests for a single variant.)

Expand Down Expand Up @@ -124,7 +145,7 @@ See the [crates.io dependencies][crates] section.
# How to structure your PR

How you prepare the commits in your PR can make a big difference for the
reviewer. Here are some tips.
reviewer. Here are some tips.

**Isolate "pure refactorings" into their own commit.** For example, if
you rename a method, then put that rename into its own commit, along
Expand Down Expand Up @@ -165,4 +186,5 @@ to the compiler.
crate-related, often the spelling is changed to `krate`.

[tcx]: ./ty.md

[crates]: ./crates-io.md
141 changes: 85 additions & 56 deletions src/tools/tidy/src/ext_tool_checks.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,74 +89,45 @@ fn check_impl(

if python_lint {
eprintln!("linting python files");
let mut cfg_args_ruff = cfg_args.clone();
let mut file_args_ruff = file_args.clone();

let mut cfg_path = root_path.to_owned();
cfg_path.extend(RUFF_CONFIG_PATH);
let mut cache_dir = outdir.to_owned();
cache_dir.extend(RUFF_CACHE_PATH);

cfg_args_ruff.extend([
"--config".as_ref(),
cfg_path.as_os_str(),
"--cache-dir".as_ref(),
cache_dir.as_os_str(),
]);

if file_args_ruff.is_empty() {
file_args_ruff.push(root_path.as_os_str());
}

let mut args = merge_args(&cfg_args_ruff, &file_args_ruff);
args.insert(0, "check".as_ref());
let res = py_runner(py_path.as_ref().unwrap(), true, None, "ruff", &args);
let py_path = py_path.as_ref().unwrap();
let res = run_ruff(root_path, outdir, py_path, &cfg_args, &file_args, &["check".as_ref()]);

if res.is_err() && show_diff {
eprintln!("\npython linting failed! Printing diff suggestions:");

args.insert(1, "--diff".as_ref());
let _ = py_runner(py_path.as_ref().unwrap(), true, None, "ruff", &args);
let _ = run_ruff(root_path, outdir, py_path, &cfg_args, &file_args, &[
"check".as_ref(),
"--diff".as_ref(),
]);
}
// Rethrow error
let _ = res?;
}

if python_fmt {
let mut cfg_args_ruff = cfg_args.clone();
let mut file_args_ruff = file_args.clone();

let mut args: Vec<&OsStr> = vec!["format".as_ref()];
if bless {
eprintln!("formatting python files");
} else {
eprintln!("checking python file formatting");
cfg_args_ruff.push("--check".as_ref());
}

let mut cfg_path = root_path.to_owned();
cfg_path.extend(RUFF_CONFIG_PATH);
let mut cache_dir = outdir.to_owned();
cache_dir.extend(RUFF_CACHE_PATH);

cfg_args_ruff.extend(["--config".as_ref(), cfg_path.as_os_str()]);

if file_args_ruff.is_empty() {
file_args_ruff.push(root_path.as_os_str());
args.push("--check".as_ref());
}

let mut args = merge_args(&cfg_args_ruff, &file_args_ruff);
args.insert(0, "format".as_ref());
let res = py_runner(py_path.as_ref().unwrap(), true, None, "ruff", &args);
let py_path = py_path.as_ref().unwrap();
let res = run_ruff(root_path, outdir, py_path, &cfg_args, &file_args, &args);

if res.is_err() && show_diff {
eprintln!("\npython formatting does not match! Printing diff:");

args.insert(0, "--diff".as_ref());
let _ = py_runner(py_path.as_ref().unwrap(), true, None, "ruff", &args);
}
if res.is_err() && !bless {
if show_diff {
eprintln!("\npython formatting does not match! Printing diff:");

let _ = run_ruff(root_path, outdir, py_path, &cfg_args, &file_args, &[
"format".as_ref(),
"--diff".as_ref(),
]);
}
eprintln!("rerun tidy with `--extra-checks=py:fmt --bless` to reformat Python code");
}

// Rethrow error
let _ = res?;
}
Expand Down Expand Up @@ -247,6 +218,38 @@ fn check_impl(
Ok(())
}

fn run_ruff(
root_path: &Path,
outdir: &Path,
py_path: &Path,
cfg_args: &[&OsStr],
file_args: &[&OsStr],
ruff_args: &[&OsStr],
) -> Result<(), Error> {
let mut cfg_args_ruff = cfg_args.into_iter().copied().collect::<Vec<_>>();
let mut file_args_ruff = file_args.into_iter().copied().collect::<Vec<_>>();

let mut cfg_path = root_path.to_owned();
cfg_path.extend(RUFF_CONFIG_PATH);
let mut cache_dir = outdir.to_owned();
cache_dir.extend(RUFF_CACHE_PATH);

cfg_args_ruff.extend([
"--config".as_ref(),
cfg_path.as_os_str(),
"--cache-dir".as_ref(),
cache_dir.as_os_str(),
]);

if file_args_ruff.is_empty() {
file_args_ruff.push(root_path.as_os_str());
}

let mut args: Vec<&OsStr> = ruff_args.into_iter().copied().collect();
args.extend(merge_args(&cfg_args_ruff, &file_args_ruff));
py_runner(py_path, true, None, "ruff", &args)
}

/// Helper to create `cfg1 cfg2 -- file1 file2` output
fn merge_args<'a>(cfg_args: &[&'a OsStr], file_args: &[&'a OsStr]) -> Vec<&'a OsStr> {
let mut args = cfg_args.to_owned();
Expand Down Expand Up @@ -321,8 +324,16 @@ fn get_or_create_venv(venv_path: &Path, src_reqs_path: &Path) -> Result<PathBuf,
fn create_venv_at_path(path: &Path) -> Result<(), Error> {
/// Preferred python versions in order. Newest to oldest then current
/// development versions
const TRY_PY: &[&str] =
&["python3.11", "python3.10", "python3.9", "python3", "python", "python3.12", "python3.13"];
const TRY_PY: &[&str] = &[
"python3.13",
"python3.12",
"python3.11",
"python3.10",
"python3.9",
"python3",
"python",
"python3.14",
];

let mut sys_py = None;
let mut found = Vec::new();
Expand Down Expand Up @@ -357,22 +368,40 @@ fn create_venv_at_path(path: &Path) -> Result<(), Error> {
return Err(ret);
};

eprintln!("creating virtual environment at '{}' using '{sys_py}'", path.display());
let out = Command::new(sys_py).args(["-m", "virtualenv"]).arg(path).output().unwrap();
// First try venv, which should be packaged in the Python3 standard library.
// If it is not available, try to create the virtual environment using the
// virtualenv package.
if try_create_venv(sys_py, path, "venv").is_ok() {
return Ok(());
}
try_create_venv(sys_py, path, "virtualenv")
}

fn try_create_venv(python: &str, path: &Path, module: &str) -> Result<(), Error> {
eprintln!(
"creating virtual environment at '{}' using '{python}' and '{module}'",
path.display()
);
let out = Command::new(python).args(["-m", module]).arg(path).output().unwrap();

if out.status.success() {
return Ok(());
}

let stderr = String::from_utf8_lossy(&out.stderr);
let err = if stderr.contains("No module named virtualenv") {
let err = if stderr.contains(&format!("No module named {module}")) {
Error::Generic(format!(
"virtualenv not found: you may need to install it \
(`{sys_py} -m pip install virtualenv`)"
r#"{module} not found: you may need to install it:
`{python} -m pip install {module}`
If you see an error about "externally managed environment" when running the above command,
either install `{module}` using your system package manager
(e.g. `sudo apt-get install {python}-{module}`) or create a virtual environment manually, install
`{module}` in it and then activate it before running tidy.
"#
))
} else {
Error::Generic(format!(
"failed to create venv at '{}' using {sys_py}: {stderr}",
"failed to create venv at '{}' using {python} -m {module}: {stderr}",
path.display()
))
};
Expand Down
Loading