Skip to content

Use a list of requirement constraints for lockfile invalidation - #16469

Merged
Eric-Arellano merged 3 commits into
pantsbuild:mainfrom
Eric-Arellano:constraints-file-dont-use-hash
Aug 10, 2022
Merged

Use a list of requirement constraints for lockfile invalidation#16469
Eric-Arellano merged 3 commits into
pantsbuild:mainfrom
Eric-Arellano:constraints-file-dont-use-hash

Conversation

@Eric-Arellano

Copy link
Copy Markdown
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
@Eric-Arellano
Eric-Arellano requested a review from chrisjrn August 10, 2022 18:09
# 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

@dataclass
class _PipArgsAndConstraintsSetup:
args: list[str]

Copy link
Copy Markdown
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
Copy Markdown
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

huonw commented Aug 10, 2022

Copy link
Copy Markdown
Contributor

Woohoo, thanks for the consideration! 😄

@Eric-Arellano

Copy link
Copy Markdown
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