Fix opam install on a local directory not updating pinned packages' metadata#6209
Conversation
ce39103 to
e8fc621
Compare
rjbou
left a comment
There was a problem hiding this comment.
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.
3a911c1 to
a95f45f
Compare
a95f45f to
5e531b5
Compare
| ### ::: 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.
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.
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.
fd9e61e to
ffc0d4f
Compare
ffc0d4f to
f5b80ec
Compare
|
rebased on master |
We need to upgrade the package to keep in sync the switch. |
f5b80ec to
294713e
Compare
src/client/opamAuxCommands.ml
Outdated
| OpamFile.OPAM.read_opt nf.pin.pin_file | ||
| with | ||
| | Some opam0, Some opam -> | ||
| (* Keep synchronised with [OpamFile.OPAM.pp_raw_fields] added |
There was a problem hiding this comment.
i'm wondering if it would be better to have a function in OpamFile.OPAM that does all these additions in order to compare (or just returns the new opam file?), like that it's easier to keep synchronised as they would be in the same module / near in the code.
There was a problem hiding this comment.
I've added two fixup commits (to cleanup of course) exploring that idea.
The first one rewrites the comparison to be more efficient (2 read instead of 4), and more correct.
In particular the available field was bugging me. The pp_minimal_opam_version function would rewrite the available field but i can't find any good reason for this.
This function was originally added in 1d32477 + ca84c3a but i can't find any reasoning for this change aside from "consistency". I personally think this makes the whole thing less consistent by introducing differences between original file and rewritten files, which is nearly impossible to detangle.
For this reason i chose to remove this part of the code which makes for a needed simplification, in my opinion.
What do you think?
There was a problem hiding this comment.
I extracted the OpamFile.OPAM function extraction idea to #6446 as i think we need more time to think about this
294713e to
e3b74f6
Compare
e3b74f6 to
f50852d
Compare
|
|
||
| No solution found, exiting | ||
| # Return code 20 # | ||
| ### :C:e: Make sure opam install works when the opam file changes and the package is pinned |
There was a problem hiding this comment.
Depending on the order of merge, these header may need a change in the last rebase (:C:e: is also used in another PR)
| | Some opam0 -> | ||
| (match OpamFile.OPAM.read_opt nf.pin.pin_file with | ||
| | Some opam -> | ||
| let opam = OpamFile.OPAM.with_locked_opt nf.pin.pin_locked opam in |
There was a problem hiding this comment.
It was discarded on purpose as to me #6438 removes the actual change made to the opam definition that we have to think about. But i've readded it, i don't mind either way.
…l opam file changes while being pinned
…ected and ensure showing local opam files is working as expected
f50852d to
e3f9de0
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.
e3f9de0 to
3b8a10e
Compare
Fixes #5567
Fixes #4507
Queued on #6438
I have no idea why
for_viewwas introduced in 9b2a67f but i've testedopam showand i'm not seeing any breaking nor does the testsuite.The bypass here was causing local packages to be put in the
already_pinnedset. However this set is only used to update the<switch>/.opam-switch/sources/<pkg>directory, not the<...>/overlaydirectory as the assumption with this set is that the package description hasn't change.Removing that bypass makes the function much more consistent.
The
reinstallbit is a bit easier to understand: when using--deps-onlyor--show-action, the codepath goes throughsimulate_local_pinningwhich didn't take into account that some packages being pinned are already pinned. In that case thereinstallfield must be updated to ensure the rest of the code knows that package should be reinstalled.