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

Use a list of requirement constraints for lockfile invalidation #16469

Merged

Conversation

Eric-Arellano
Copy link
Contributor

Improves upon #16420. @huonw wisely suggested we avoid using hash for constraints files because it makes Git diffs and merge conflicts much worse. python-poetry/poetry#496 rust-lang/cargo#7070

[ci skip-rust]
[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]
@Eric-Arellano Eric-Arellano added the category:internal CI, fixes for not-yet-released features, etc. label Aug 10, 2022
# 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]
@Eric-Arellano Eric-Arellano marked this pull request as ready for review August 10, 2022 19:15
@@ -155,35 +154,41 @@ def warn_python_repos(option: str) -> None:
return MaybeWarnPythonRepos()


@dataclass
class _PipArgsAndConstraintsSetup:
args: list[str]
Copy link
Contributor

Choose a reason for hiding this comment

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

This works since _PipArgsAndConstraintsSetup is private and only a return type of a helper, but having to look at any un-frozen struct, at least for me, raises suspicion reading rule code. If there was some mysterious bug reported and I was scanning the codebase, this would cause me to burn un-needed time reviewing why this is OK. I may be a poor example here though of a code reader.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True. I myself was a bit confused trying to trace whether this was safe or not. Thanks

# 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]
@Eric-Arellano Eric-Arellano enabled auto-merge (squash) August 10, 2022 19:49
@Eric-Arellano Eric-Arellano merged commit 318fea7 into pantsbuild:main Aug 10, 2022
@Eric-Arellano Eric-Arellano deleted the constraints-file-dont-use-hash branch August 10, 2022 20:30
@huonw
Copy link
Contributor

huonw commented Aug 10, 2022

Woohoo, thanks for the consideration! 😄

@Eric-Arellano
Copy link
Contributor Author

And thank you for the recommendation! It was a great one, and it shed light on 2 other uncertainties I had re: lockfile metadata for this project :D

cczona pushed a commit to cczona/pants that referenced this pull request Sep 1, 2022
…sbuild#16469)

Improves upon pantsbuild#16420. @huonw wisely suggested we avoid using `hash` for constraints files because it makes Git diffs and merge conflicts much worse. python-poetry/poetry#496 rust-lang/cargo#7070

[ci skip-rust]
[ci skip-build-wheels]
@huonw huonw mentioned this pull request Jul 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:internal CI, fixes for not-yet-released features, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants