-
Notifications
You must be signed in to change notification settings - Fork 359
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
Fix "opam install <local_dir>" not updating pinned packages' metadata #6209
base: master
Are you sure you want to change the base?
Fix "opam install <local_dir>" not updating pinned packages' metadata #6209
Conversation
ce39103
to
e8fc621
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reviewed commit per commit, it's better to look at comments per commit.
I'm not sure about the third commit (reinstall packages), it raises a good question. we want to update the opam file internally, but do we want to upgrade the package itself? Usually, when users use --deps-only
they don't want to launch an install of given package (no need, or don't want to double compile it). When the package is already installed, there is the "we want to have a consistent state" that leads us to want to upgrade the package. From CLI perspective, it is more consistent to not upgrade the package.
Maybe a note or warning to highlight the situation package updated internally, if you really want to reinstall it, launch opam install pkg is better than a full reinstall triggered by --deps-only
.
@@ -303,10 +303,7 @@ let autopin_aux st ?quiet ?(for_view=false) ?recurse ?subpath ?locked | |||
| _ -> false) | |||
| None -> false) | |||
&& | |||
(* For `opam show`, we need to check does the opam file changed to | |||
perform a simulated pin if so *) | |||
(not for_view || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change will trigger a OpamPinCommad.source_pin
call that retrieves the whole surce, while what we want here is to update the opam file only.
master_changes.md
Outdated
@@ -25,6 +25,7 @@ users) | |||
|
|||
## Install | |||
* Fix `opam install <local_dir>` not updating pinned packages' metadata [#6209 @kit-ty-kate - fix #5567] | |||
* Fix `opam install --deps-only/--show-action <local_dir>` not updating pinned packages' metadata [#6209 @kit-ty-kate - fix #5567] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, the PR name should change to something more general, like fix opam install when targeting a local directory sometimes not updating pinned packages' metadata, it is no more only about local dir as a target.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is it not only about local dir?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My mistake, I meant that it is not only the command opam install <local-dir>
but also combined with option --deps-only
and --show-action
that is fixed here. I updated the proposal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about "Fix opam install on a local directory not updating pinned packages' metadata" ?
e8fc621
to
baafe9e
Compare
…l opam file changes while being pinned
baafe9e
to
53418ca
Compare
3a911c1
to
a95f45f
Compare
…ages' metadata The reinstall field from the switch state wasn't updated according to the implicit pin and thus opam didn't think there was anything to do.
a95f45f
to
5e531b5
Compare
let opam = | ||
(* This is required to avoid the case where locked opam files were | ||
stored with the added `available: opam-version >= "2.1"` *) | ||
OpamFile.OPAM.with_available (OpamFile.OPAM.available opam0) opam |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It worth adding a comment where these fields are added for the internal stored opam, in order to update this piece of code when it is changed.
I don't know if we can have a test to follow that, keep the consistency between these fields added, and the ones checked here.
depends: "dependency" {= "1"} | ||
### ::: behaviour when the package is not pinned | ||
### opam install ./pin-change | ||
pin-change is now pinned to git+file://${BASEDIR}/pin-change#master (version dev) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't want the repin note, it is not repinned to another url (no source fetching as source_pin does), only the opam file updated/changed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i general i would say yes, however this is not the role of this PR. This behaviour is the one that opam currently has and this PR is not changing it.
See for example these two lines where a foo-core and foo-format were repinned and the "is now pinned to" message still appears:
opam/tests/reftests/autopin.test
Line 231 in f3466d9
### opam pin opam/tests/reftests/autopin.test
Line 259 in f3466d9
### opam pin -n ./foo
### ::: behaviour when the package is not pinned | ||
### opam install ./pin-change | ||
pin-change is now pinned to git+file://${BASEDIR}/pin-change#master (version dev) | ||
[NOTE] Ignoring uncommitted changes in ${BASEDIR}/pin-change (`--working-dir' not specified or specified with no argument). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we have a more fine grained handling of pin update (opam file vs source update), we can even remove that note if only the uncommitted opam file is updated, as now we have the information.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as with https://github.com/ocaml/opam/pull/6209/files#r1815454356 i believe. I think those two improvements can be their own tickets to be fixed later.
@@ -303,16 +303,31 @@ let autopin_aux st ?quiet ?(for_view=false) ?recurse ?subpath ?locked | |||
| _ -> false) | |||
| None -> false) | |||
&& | |||
(* For `opam show`, we need to check does the opam file changed to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It worth adding a test for opam show
that highlights that there is no change in the behaviour
Fixes #5567
I have no idea why
for_view
was introduced in kit-ty-kate@9b2a67f but i've testedopam show
and i'm not seeing any breaking nor does the testsuite.The bypass here was causing local packages to be put in the
already_pinned
set. However this set is only used to update the<switch>/.opam-switch/sources/<pkg>
directory, not the<...>/overlay
directory as the assumption with this set is that the package description hasn't change.Removing that bypass makes the function much more consistent.
The
reinstall
bit is a bit easier to understand: when using--deps-only
or--show-action
, the codepath goes throughsimulate_local_pinning
which didn't take into account that some packages being pinned are already pinned. In that case thereinstall
field must be updated to ensure the rest of the code knows that package should be reinstalled.