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

On version pinning don't read archive opam file #4931

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

rjbou
Copy link
Collaborator

@rjbou rjbou commented Nov 24, 2021

fix #4608

@rjbou rjbou added this to the 2.2.0~alpha milestone Nov 24, 2021
OpamFile.OPAM.safe_read >>=
OpamFile.OPAM.version_opt)
= Some nv.version
(OpamSwitchState.is_version_pinned st (OpamPackage.name nv) ||
Copy link
Member

Choose a reason for hiding this comment

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

Does this also return true when doing opam pin add pkg.version <repo>? In that case, there is no need to look into the archive/repo either

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.

I’m not sure what the entire if statement is trying to do in the first place.

"do not do anything (equivalent to --no-action) if e.g. doing something like opam pin add pkg.version <archive/repo> and the version number is the same as the one in the archive/repo" ? That seems a bit suspicious to me

@kit-ty-kate
Copy link
Member

I had a go at trying to debug this suspicious if and i have a repro case:

$ cat test/dune.opam
opam-version: "2.0"
version: "2.0.1"
build: ["sh" "-c" "ls && false"]
url {
  src: "https://github.com/ocaml/dune/archive/main.tar.gz"
}
$ opam pin add dune.2.0.1 test/
[...]

#=== ERROR while compiling dune.2.0.1 =========================================#
# context     2.1.0 | linux/x86_64 | ocaml-variants.4.13.0+trunk | pinned(file:///home/opam/test)
# path        ~/.opam/4.13/.opam-switch/build/dune.2.0.1
# command     /bin/sh -c ls && false
# exit-code   1
# env-file    ~/.opam/log/dune-236-a0af0d.env
# output-file ~/.opam/log/dune-236-a0af0d.out
### output ###
# dune.opam

So I really think this whole if statement should be removed as a whole.

@rjbou rjbou added the PR: WIP Not for merge at this stage label Nov 29, 2021
@rjbou
Copy link
Collaborator Author

rjbou commented Nov 29, 2021

It is not suspicious, that if statement is present for a reason: optimisation. If the package is version pinned, there is no need to retrieve it again / synchronise it. Maybe we can say/decide that this optimisation is not efficient, but that's another question.
opam pin add pkg.version <non-archive-url> is different than a version pin. In the first case, it is a path/git/etc pin with given version, while a version pin is "pin pkg with repo opam file at that version", so sources are taken from url archive (or repo, on official opam repository we take only archives url but they can be vcs ones), and is not prone to change/update unless the opam repository changes.
So if you opam pin add pkg.version url, it pins to path/vcs url, it takes the opam file from the url, and it needs to synchronise from it each time. The dune.2.0.1 pinning/install behavior is the wanted one.

We need to check opam pin pkg.version archive-url behavior, and see what we want for that.

We also need to add some pinning and downloading/syncing tests, to check and be sure that we don't sync more that we need.

"do not do anything (equivalent to --no-action) if e.g. doing something like opam pin add pkg.version <archive/repo> and the version number is the same as the one in the archive/repo"

From where is this citation? Maybe we need to clarify that doc.

@rjbou rjbou marked this pull request as draft December 1, 2021 15:02
@rjbou rjbou assigned rjbou and unassigned rjbou Dec 1, 2021
@dra27 dra27 removed this from the 2.2.0~alpha milestone Feb 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: WIP Not for merge at this stage
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove the need to read the opam file inside the archive on a version pin
3 participants