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

Turn --verify-hashes on by default #9170

Merged
merged 11 commits into from
Nov 18, 2024
Merged

Conversation

hauntsaninja
Copy link
Contributor

Fixes #9164

Using clap's default_value_t makes the flag function unhappy, so just set the default when we unwrap. Tested with no flags, --verify-hashes, --no-verify-hashes and setting in uv.toml

Comment on lines +1552 to +1557
creating src/project.egg-info
writing src/project.egg-info/PKG-INFO
writing dependency_links to src/project.egg-info/dependency_links.txt
writing requirements to src/project.egg-info/requires.txt
writing top-level names to src/project.egg-info/top_level.txt
writing manifest file 'src/project.egg-info/SOURCES.txt'
Copy link
Contributor Author

@hauntsaninja hauntsaninja Nov 17, 2024

Choose a reason for hiding this comment

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

I don't understand what's causing these two new lines

@hauntsaninja
Copy link
Contributor Author

hauntsaninja commented Nov 17, 2024

Note that UV_VERIFY_HASHES=0 won't really do anything because flag will receive false, false and turn it into None -> true, so we'll continue to verify hashes. We need something that does the opposite of BoolishValueParser

@charliermarsh charliermarsh self-assigned this Nov 17, 2024
@charliermarsh
Copy link
Member

What if we change HashCheckingMode::from_args to take two Option? Then we can differentiate between UV_VERIFY_HASHES being 0, 1, or unset.

It would also be somewhat conventional for us to make --verify-hashes hidden, and add UV_NO_VERIFY_HASHES.

@hauntsaninja
Copy link
Contributor Author

hauntsaninja commented Nov 17, 2024

I assume you meant flag, not HashCheckingMode::from_args. Right now it's too late by then, since clap will already have given you bools. The naive thing of turning verify_hashes into an Option<bool> in the CLI struct seemed to mess with clap's logic for flags and setting default_missing_value didn't seem to fix. I'm probably missing some dumb clap trick, feel free to take over this PR if it's obvious to you!

@charliermarsh
Copy link
Member

Sounds good, I'll see what I can do...

@hauntsaninja
Copy link
Contributor Author

hauntsaninja commented Nov 17, 2024

Okay, the issue is that default_missing_value just silently doesn't work if you don't also specify require_equals and num_args. I'll push the change (if you see a better way feel free to)!

@charliermarsh
Copy link
Member

Okay I modified things a bit such that instead of UV_VERIFY_HASHES=0, we require UV_NO_VERIFY_HASHES=1, which is consistent with the rest of the CLI. We also now document --no-verify-hashes instead of --verify-hashes.

@charliermarsh charliermarsh enabled auto-merge (squash) November 18, 2024 01:50
@charliermarsh charliermarsh merged commit 71d9c45 into astral-sh:main Nov 18, 2024
35 checks passed
@hauntsaninja hauntsaninja deleted the uv-hashes branch November 18, 2024 02:31
pub const UV_VERIFY_HASHES: &'static str = "UV_VERIFY_HASHES";
/// Equivalent to the `--no-verify-hashes` argument. Disables hash verification for
/// `requirements.txt` files.
pub const UV_NO_VERIFY_HASHES: &'static str = "UV_VERIFY_HASHES";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Need to change the string constant here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Hah, great... Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Turn --verify-hashes on by default
2 participants