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
Validate Python user lockfiles & improve tool lockfile error message #14353
Validate Python user lockfiles & improve tool lockfile error message #14353
Conversation
# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
Validation is now enabled for user lockfiles! Only the error messages are bad. # Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
This will help with error messages # Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Each commit is distinct, but this is probably easiest enough to review in a single pass.
Thanks @chrisjrn for laying the foundation for this!
Question, do we want to list what the relevant input targets were? I know how to implement it, only a question if we want it. I figure more diagnostics are better? |
@frozen_after_init | ||
@dataclass(unsafe_hash=True) | ||
class _RepositoryPexRequest: | ||
addresses: Addresses | ||
requirements: PexRequirements |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be good for docstring or field naming to clarify that both addresses
and requirements
are subsets (afaict) of the entire resolve, only used for staleness checks.
My first read of this (without thinking too much about the review title) was that we were passing in all of the requirements that made up of the resolve (which felt upside down, and I was about to comment to that effect).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed the names in general are confusing. I think Lockfile(req_strings=)
is also confusing. I'm going to punt on this for a holistic rename: #14388
It's a little tricky that interpreter constraints do change how the _RepositoryPex is used, and addresses
is used to determine those.
I don't think that that's necessary. The next step after detecting a stale lockfile (I think?) will almost always be regenerating the lockfile, rather than editing a target. |
Checks that interpreter constraints + requirements of the current run are compatible with the lockfile.
This also improves the error message for tool lockfiles to say how the resolves diverge: