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.

Sign up for GitHub

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

Jump to bottom

source: add --no-checksums & --require-checksums flags to harmonise with opam install (fetch options) #5563

Merged
merged 2 commits into from
Dec 4, 2024
Merged

source: add --no-checksums & --require-checksums flags to harmonise with opam install (fetch options) #5563

merged 2 commits into from
Dec 4, 2024

Conversation

rjbou
Copy link
Collaborator

@rjbou rjbou commented May 26, 2023 •
edited
Loading

@rjbou rjbou added the PR: QUEUED Pending pull request, waiting for other work to be merged or closed label May 26, 2023
@rjbou rjbou force-pushed the source-install branch from 22a435d to 3ba8350 Compare May 30, 2023 08:31
@rjbou rjbou requested a review from kit-ty-kate August 29, 2023 17:32
@rjbou rjbou added PR: WAITING FOR REVIEW and removed PR: QUEUED Pending pull request, waiting for other work to be merged or closed labels Aug 29, 2023
AltGr
AltGr approved these changes Sep 7, 2023
View reviewed changes
src/client/opamCommands.ml Outdated
mk_flag ~cli (cli_from cli2_2) ["require-checksums"]
"Enforce checksum verification befor downloading sources.\
This is equivalent to setting $(b,\\$OPAMREQUIRECHECKSUMS) to \"true\"."
in
Copy link
Member

Choose a reason for hiding this comment

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

might be worth sharing the flags definitions with the ones used in OpamArg.build_options ? (maybe that was too tedious to be worth it, in that case it's alright)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It is possible to share it easily with build_options, (cf. d69eb84).
I've tried to move them from build options to global options, but we loose the ability to notify that some new commands now permit and use --*-checksum options (no possibility to describe cli validity fine grained). Cf ef3b1e2

@hannesm
Copy link
Member

hannesm commented Nov 7, 2023

Appreciated, and this looks great to me. As @AltGr mentioned, it may be useful to share some code. Thanks a lot.

@kit-ty-kate kit-ty-kate removed their request for review May 9, 2024 13:24
@rjbou rjbou added this to the 2.3 milestone Jun 27, 2024
@rjbou rjbou marked this pull request as draft July 10, 2024 13:56
@dra27 dra27 modified the milestones: 2.3.0~alpha, 2.4.0~alpha1 Sep 3, 2024
@rjbou rjbou requested review from kit-ty-kate and dra27 November 22, 2024 16:30
@rjbou rjbou marked this pull request as ready for review November 22, 2024 16:30
kit-ty-kate
kit-ty-kate reviewed Nov 22, 2024
View reviewed changes
Copy link
Member

@kit-ty-kate kit-ty-kate left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I don't understand this feature request. What does this bring in terms of change for users?

I understand --require-checksums, although i feel like this would have more usability as a global opam option.
But --no-checksums i really don't understand. If the goal is to allow packages without a checksum, then why not simply add a --allow-no-checksums argument instead of ignoring the checksum altogether?

I'm also wondering what how opam source is special compared to the other commands that might fetch packages.

@rjbou
Copy link
Collaborator Author

rjbou commented Nov 26, 2024

As discussed in dev meeting, the idea is to harmonise option for opam source, make --no-checksum and --require-checksum option available.
related #6311 & #6312

@rjbou rjbou changed the title source: add --no-checksums & --require-checksums flags source: add --no-checksums & --require-checksums flags to harmonise with opam install (build options) Dec 4, 2024
@rjbou rjbou requested a review from kit-ty-kate December 4, 2024 11:29
@rjbou rjbou requested review from AltGr and removed request for dra27 and AltGr December 4, 2024 11:30
@kit-ty-kate kit-ty-kate changed the title source: add --no-checksums & --require-checksums flags to harmonise with opam install (build options) source: add --no-checksums & --require-checksums flags to harmonise with opam install (fetch options) Dec 4, 2024
@rjbou rjbou merged commit 75f9eec into ocaml:master Dec 4, 2024
42 checks passed
@rjbou rjbou mentioned this pull request Dec 4, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AltGr AltGr AltGr approved these changes

@kit-ty-kate kit-ty-kate kit-ty-kate approved these changes

Assignees
No one assigned
Labels
STATE: READY TO MERGE
Projects
None yet
Milestone
2.4.0~alpha1
Development

Successfully merging this pull request may close these issues.
5 participants
@rjbou @hannesm @AltGr @kit-ty-kate @dra27