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.

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

Add ruff version with long version display #8034

Merged
merged 13 commits into from
Oct 20, 2023
Merged

Add ruff version with long version display #8034

merged 13 commits into from
Oct 20, 2023

Conversation

zanieb
Copy link
Member

@zanieb zanieb commented Oct 18, 2023

Adds a new ruff version sub-command which displays long version information in the style of cargo and rustc. We include the number of commits since the last release tag if its a development build, in the style of Python's versioneer.

❯ ruff version
ruff 0.1.0+14 (947940e91 2023-10-18)
❯ ruff version --output-format json
{
  "version": "0.1.0",
  "commit_info": {
    "short_commit_hash": "947940e91",
    "commit_hash": "947940e91269f20f6b3f8f8c7c63f8e914680e80",
    "commit_date": "2023-10-18",
    "last_tag": "v0.1.0",
    "commits_since_last_tag": 14
  }
}%
❯ cargo version
cargo 1.72.1 (103a7ff2e 2023-08-15)

Test plan

I've tested this manually locally, but want to at least add unit tests for the message formatting. We'd also want to check the next release to ensure the information is correct.

I checked build behavior with a detached head and branches.

Future work

We could include rustc and cargo versions from the build, the current Python version, and other diagnostic information for bug reports.

The --version and -V output is unchanged. However, we could update it to display the long ruff version without the rust and cargo versions (this is what cargo does). We'll need to be careful to ensure this does not break downstream packages which parse our version string.

❯ ruff --version
ruff 0.1.0

The LSP should be updated to use ruff version --output-format json instead of parsing ruff --version.

@zanieb zanieb force-pushed the zanie/long-version branch from b65796d to 9853a22 Compare October 18, 2023 02:01
@zanieb
Copy link
Member Author

zanieb commented Oct 18, 2023

I'm not sure I'm into shadow-rs for this — I think I may prefer to do something like cargo's implementation.

@charliermarsh
Copy link
Member

I haven't looked at the code yet, but one thing to note is that we do parse the ruff version output in the LSP:

from packaging.version import Version


def version(executable: str) -> Version:
    """Returns the version of the executable at the given path."""
    output = subprocess.check_output([executable, "--version"]).decode().strip()
    version = output.replace("ruff ", "")  # no removeprefix in 3.7 :/
    return Version(version)

@zanieb
Copy link
Member Author

zanieb commented Oct 18, 2023

This doesn't change any existing behavior — great note though it may be a good reason not to change --version to be long-form.

@konstin
Copy link
Member

konstin commented Oct 18, 2023

I'd like to switch the lsp to use the json instead

@zanieb
Copy link
Member Author

zanieb commented Oct 18, 2023

That's a great idea!

@zanieb
Copy link
Member Author

zanieb commented Oct 18, 2023

I'm really pleased with the handwritten implementation. I adapted the version and commit information from https://github.com/zanieb/poetry-relax/blob/main/scripts/version, https://github.com/rust-lang/cargo/blob/master/build.rs, and https://github.com/rust-lang/cargo/blob/master/src/cargo/version.rs

@github-actions
Copy link
Contributor

github-actions bot commented Oct 18, 2023

PR Check Results

Ecosystem

✅ ecosystem check detected no changes.

@MichaReiser
Copy link
Member

I'd like to switch the lsp to use the json instead

How would we detect if the ruff version is new enough to support the new json output without using --version first?

@charliermarsh
Copy link
Member

Hah yes that’s true. Maintaining compatibility between Ruff and the LSP feels increasingly annoying.

@zanieb
Copy link
Member Author

zanieb commented Oct 18, 2023

I'd like to switch the lsp to use the json instead

How would we detect if the ruff version is new enough to support the new json output without using --version first?

True, that's pretty funny. We can go the "try and fail" way at the cost of some speed.

@konstin
Copy link
Member

konstin commented Oct 19, 2023

How would we detect if the ruff version is new enough to support the new json output without using --version first?

We don't, we have to wait until a ruff with a json version is the oldest supported version plus a bit of buffer for better error messages.

Comment on lines 23 to 45
let git_head_path = git_dir.join("HEAD");
println!(
"cargo:rerun-if-changed={}",
git_head_path.as_path().display()
);

let git_head_contents = fs::read_to_string(git_head_path);
if let Ok(git_head_contents) = git_head_contents {
// The contents are either a commit or a reference in the following formats
// "<commit>""
// "ref <ref>""
// If a commit, checking if the HEAD file has changed is sufficient
// If a ref we want to add the head file for that ref to rebuild on commit
let mut git_ref_parts = git_head_contents.split_whitespace();
git_ref_parts.next();
if let Some(git_ref) = git_ref_parts.next() {
let git_ref_path = git_dir.join(git_ref);
println!(
"cargo:rerun-if-changed={}",
git_ref_path.as_path().display()
);
}
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Okay this is a little bit much but i tested this out and basically without this, if you change any file in the ruff_cli crate e.g. add a file foo to the crate root then a rebuild is forced — this is entirely unnecessary so, here, if a git directory is present we will use commit information from it to determine if rebuilding should occur.

@zanieb
Copy link
Member Author

zanieb commented Oct 19, 2023

Just needs some snapshot tests for the version formatting now.

@zanieb
Copy link
Member Author

zanieb commented Oct 20, 2023

I wish we could test the ruff version command output but it would change per commit and redacting it would be meaningless.

@zanieb zanieb marked this pull request as ready for review October 20, 2023 15:35
@zanieb zanieb merged commit 860ffb9 into main Oct 20, 2023
16 checks passed
@zanieb zanieb deleted the zanie/long-version branch October 20, 2023 19:07
zanieb added a commit that referenced this pull request Oct 23, 2023
Adds a CI job which runs `ruff-lsp` tests against the current Ruff
build.

Avoids rebuilding Ruff at the cost of running _after_ the cargo tests
have finished. Might be worth the rebuild to get earlier feedback but I
don't expect it to fail often?

xref astral-sh/ruff-lsp#286

## Test plan

Verified use of the development version by inspecting version output in
CI; supported by astral-sh/ruff-lsp#289 and
#8034
zanieb added a commit to astral-sh/uv that referenced this pull request Feb 23, 2024
Similar to astral-sh/ruff#8034

Adds more version information so it's clear what revision the user is on

```
❯ cargo run -q -- --version
uv 0.1.10 (daa8565 2024-02-23)
❯ cargo run -q -- -V
uv 0.1.10
❯ cargo run -q -- version
uv 0.1.10 (daa8565 2024-02-23)
❯ cargo run -q -- version --output-format json
{
  "version": "0.1.10",
  "commit_info": {
    "short_commit_hash": "daa8565a7",
    "commit_hash": "daa8565a75249305821fdc34ace085060c082ba3",
    "commit_date": "2024-02-23",
    "last_tag": null,
    "commits_since_last_tag": 0
  }
}
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants