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

Stop populating opam file with extra-files #5564

Merged
merged 2 commits into from
Sep 19, 2024

Conversation

rjbou
Copy link
Collaborator

@rjbou rjbou commented May 26, 2023

Current behaviour is at lint stage, if an extra-files is found in files directory, but not in the opam file, it is added in the opam file automatically.
This behaviour is removed, all extra-files need to be declared in opam file.

/cc @hannesm @reynir

@rjbou rjbou added the PR: QUEUED Pending pull request, waiting for other work to be merged or closed label May 26, 2023
@rjbou
Copy link
Collaborator Author

rjbou commented Jul 26, 2023

fix #5013

Copy link
Member

@AltGr AltGr left a comment

Choose a reason for hiding this comment

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

Good, this ends a "transition period" that was really much too long :)

@hannesm
Copy link
Member

hannesm commented Nov 7, 2023

looks good to me, but I'd as well be happy if you point me to the rebased PR once #5561 is merged :)

@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
@rjbou rjbou marked this pull request as ready for review August 28, 2024 16:19
@rjbou rjbou changed the title No more populate opam file with extra-files lint: add errors about duplicated extra-files and checksums & when extra-file paths contains '..' Sep 3, 2024
@rjbou rjbou changed the title lint: add errors about duplicated extra-files and checksums & when extra-file paths contains '..' No more populate opam file with extra-files Sep 3, 2024
@rjbou rjbou changed the title No more populate opam file with extra-files Stop populating opam file with extra-files Sep 3, 2024

## Repository
* Mitigate curl/curl#13845 by falling back from --write-out to --fail if exit code 43 is returned by curl [#6168 @dra27 - fix #6120]
* When loading a repository, no more automatically populate `extra-files:` field with found files in `files/` [#5564 @rjbou]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Suggested change
* When loading a repository, no more automatically populate `extra-files:` field with found files in `files/` [#5564 @rjbou]
* When loading a repository, stop automatically populate `extra-files:` field with found files in `files/` [#5564 @rjbou]

@rjbou
Copy link
Collaborator Author

rjbou commented Sep 3, 2024

rebased

Copy link
Member

@kit-ty-kate kit-ty-kate left a comment

Choose a reason for hiding this comment

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

lgtm otherwise

@@ -249,6 +251,8 @@ users)
## opam-state
* `OpamStateConfig.opamroot_with_provenance`: restore previous behaviour to `OpamStateConfig.opamroot` for compatibility with third party code [#6047 @dra27]
* `OpamSwitchState.{,reverse_}dependencies`: make `unavailable` a non-optional argument to enforce speedups when availability information is not needed [#5317 @kit-ty-kate]
* `OpamFilteTools.add_aux_files`: ignore non registered extra-files [#5564 @@rjbou]
Copy link
Member

Choose a reason for hiding this comment

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

the ""new"" optional argument should be added to the changelog

tests/reftests/legacy-git.test Outdated Show resolved Hide resolved
tests/reftests/legacy-local.test Outdated Show resolved Hide resolved
tests/reftests/repository.test Outdated Show resolved Hide resolved
@kit-ty-kate kit-ty-kate removed the PR: QUEUED Pending pull request, waiting for other work to be merged or closed label Sep 5, 2024
master_changes.md Outdated Show resolved Hide resolved
@rjbou
Copy link
Collaborator Author

rjbou commented Sep 6, 2024

Updated opam-rt, should be good by now. The corresponding PR in opam-rt (ocaml-opam/opam-rt#78) should be merged in the same time.

@rjbou
Copy link
Collaborator Author

rjbou commented Sep 17, 2024

Tests fixed!

@kit-ty-kate
Copy link
Member

updated

Copy link
Member

@dra27 dra27 left a comment

Choose a reason for hiding this comment

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

Some suggestions for the re-wording in the manual - I find it confusing that we refer to the field as being mandatory, when what we're really meaning is that referring to each file in the field is mandatory.

I'm uneasy that we change this behaviour with no backwards compatibility - i.e. there are (CI) workflows which work today with opam 2.2 which unconditionally stop working with opam 2.3. That said, from a security standpoint it's obviously much better and the fix required for 2.3 compatibility is obviously completely compatible with older versions. So let's just ensure that the announcements for 2.3 make that crystal clear!

doc/pages/Manual.md Outdated Show resolved Hide resolved
doc/pages/Manual.md Outdated Show resolved Hide resolved
doc/pages/Manual.md Outdated Show resolved Hide resolved
@rjbou
Copy link
Collaborator Author

rjbou commented Sep 18, 2024

I'm uneasy that we change this behaviour with no backwards compatibility - i.e. there are (CI) workflows which work today with opam 2.2 which unconditionally stop working with opam 2.3.

It was at the origin a temporary thing, to transition. But it stayed so long that it became kind of as a feature.

That said, from a security standpoint it's obviously much better and the fix required for 2.3 compatibility is obviously completely compatible with older versions. So let's just ensure that the announcements for 2.3 make that crystal clear!

Yes, plus there is the command opam admin update-extrafiles that automates repository updates.

@kit-ty-kate
Copy link
Member

Thanks a lot!

@kit-ty-kate kit-ty-kate merged commit 6d05a4e into ocaml:master Sep 19, 2024
29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants