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

Add the announcement for opam 2.3.0~alpha1 #105

Merged
merged 5 commits into from
Sep 23, 2024

Conversation

kit-ty-kate
Copy link
Member

No description provided.

```
or from PowerShell for Windows systems
```
Invoke-Expression "& { $(Invoke-RestMethod https://raw.githubusercontent.com/ocaml/opam/master/shell/install.ps1) } -Version 2.3.0~alpha1"
Copy link
Member Author

Choose a reason for hiding this comment

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

Tested successfully

opam-2-3-0-alpha1.md Outdated Show resolved Hide resolved

## Major breaking change

When loading a repository, opam won't automatically populate the `extra-files:` field with the files found in the `files/` directory anymore.
Copy link
Contributor

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, opam won't automatically populate the `extra-files:` field with the files found in the `files/` directory anymore.
When loading a repository, opam won't automatically populate the `extra-files:` field with the files found in the `files/` directory anymore. This was done to add more security as the existing checksum is checked against the downloaded file.

Something like that, to add the information about the "why" of the change.

Copy link
Member Author

Choose a reason for hiding this comment

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

mmh, i disagree with the reasoning here. I don't think there is any "security" reason per say for this change.
The directory are part of the opam repository or source repository already and are as checked as are the opam files.

The purpose for it is a desire for simplification and evolution to something like ocaml/opam#5811. Removing unchecked files makes it easier to have an easily checkable opam admin migrate-extrafiles (ocaml/opam#5960). Aiming to remove the extra-files is good to make the repository leaner and avoid file duplication.

Copy link
Member Author

Choose a reason for hiding this comment

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

What about:

Suggested change
When loading a repository, opam won't automatically populate the `extra-files:` field with the files found in the `files/` directory anymore.
When loading a repository, opam won't automatically populate the `extra-files:` field with the files found in the `files/` directory anymore. This was done as a first step towards the removal of the `extra-files` field in the future and to simplify the opam specification where we hope the opam file to be the only thing that you have to look at when reading a package specification. The optionality of the `extra-files:` field would go against that principle.

Copy link
Contributor

Choose a reason for hiding this comment

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

The origin of the removal of the automation is the "security/integrity" issue: if you have a repository in the file system and the extra-file is corrupted (io or malicious), opam would load it as is, ie compute the checksum at loading and add it in the opam file. It was that behaviour that was targeted at the time.
Discussions about removing extra-files came afterwards.

Copy link
Contributor

Choose a reason for hiding this comment

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

Plus, I don't know if we decided something on removal of extra-files, it is an open discussion.

Copy link
Member Author

Choose a reason for hiding this comment

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

if you have a repository in the file system and the extra-file is corrupted (io or malicious)

For malicious corruption it would be much easier to simply corrupt the opam file directly instead, and for regular corruption opam files are also susceptible to that so while it reduces the surface area for potential corruption i find that reason a little light and i don't think it would be compelling enough for users to understand why this has been done.

Discussions about removing extra-files came afterwards.

i know i know, but i think users don't care much about the original reason. It would be more interesting to give a current compelling reason for them to understand the upsides of the breaking change.

Copy link
Member Author

Choose a reason for hiding this comment

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

Plus, I don't know if we decided something on removal of extra-files, it is an open discussion.

Sure, fair enough. What about this instead (removed that part and added a bit about file corruption):

Suggested change
When loading a repository, opam won't automatically populate the `extra-files:` field with the files found in the `files/` directory anymore.
When loading a repository, opam won't automatically populate the `extra-files:` field with the files found in the `files/` directory anymore. This was done to simplify the opam specification where we hope the opam file to be the only thing that you have to look at when reading a package specification. The optionality of the `extra-files:` field goes against that principle. This change also reduces the surface area for potential file corruption as it ensures that extra-files are checked against their checksums.

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.

Various thoughts and nits - but nothing particularly major, thanks!

opam-2-3-0-alpha1.md Outdated Show resolved Hide resolved
opam-2-3-0-alpha1.md Outdated Show resolved Hide resolved
opam-2-3-0-alpha1.md Outdated Show resolved Hide resolved
opam-2-3-0-alpha1.md Outdated Show resolved Hide resolved
opam-2-3-0-alpha1.md Outdated Show resolved Hide resolved
opam-2-3-0-alpha1.md Outdated Show resolved Hide resolved
opam-2-3-0-alpha1.md Outdated Show resolved Hide resolved
opam-2-3-0-alpha1.md Outdated Show resolved Hide resolved
opam-2-3-0-alpha1.md Outdated Show resolved Hide resolved
opam-2-3-0-alpha1.md Outdated Show resolved Hide resolved
kit-ty-kate and others added 4 commits September 23, 2024 13:38
Co-authored-by: David Allsopp <david.allsopp@metastack.com>
Co-authored-by: David Allsopp <david.allsopp@metastack.com>
@kit-ty-kate kit-ty-kate merged commit 9557b8d into ocaml:master Sep 23, 2024
@kit-ty-kate kit-ty-kate deleted the opam-2.3.0-alpha1 branch September 23, 2024 12:51
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.

3 participants