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

Invalidate plugins at root upgrade #4619

Closed
dra27 opened this issue Mar 30, 2021 · 4 comments · Fixed by #4641
Closed

Invalidate plugins at root upgrade #4619

dra27 opened this issue Mar 30, 2021 · 4 comments · Fixed by #4641
Milestone

Comments

@dra27
Copy link
Member

dra27 commented Mar 30, 2021

This Dockerfile fails at the final step:

FROM ocaml/opam:debian-ocaml-4.12
WORKDIR /src
RUN opam install dune
RUN sudo chown opam /src
RUN opam exec -- dune init project foo
RUN echo > dune-project '(lang dune 2.8)\n(generate_opam_files true)(source (github foo/bar))(package (name foo))'
RUN opam exec -- dune build
RUN opam install opam-state.2.0.8
RUN opam install opam-dune-lint --deps-only
RUN opam pin -yn opam-dune-lint 'https://github.com/ocurrent/opam-dune-lint.git#790f4d552d37cfeb6c13c5d1d543ef7da576ad1d'
RUN opam dune-lint
RUN sudo rm /usr/bin/opam && sudo ln /usr/bin/opam-2.1 /usr/bin/opam
RUN opam update
RUN opam dune-lint

The reason is that opam-dune-lint was compiled with opam-state 2.0.x and then the root gets upgraded. ocurrent/opam-dune-lint#26 proposes fixing its dependencies to require opam-state at least matching the opam version. This fixes manually installing opam-state 2.0.x in a 2.1 root by forcing the plugin to be uninstalled (cf. ocurrent/opam-dune-lint#26 (comment)), but I think we need to do something about the upgrade case too.

A simple option would be to delete all the symlinks in plugins/bin as part of the format upgrade, but that doesn't work in this case where the plugin is installed in the current switch. More complex solutions might include a validity check on the dependencies of a plugin (possibly just once after upgrade?) - if the pin is changed to bf451c8cf2d4f2be3c7d979b613c79588b55a172, then opam-dune-lint definitely needs recompiling after the root upgrade (opam update will catch that) but it would also be detectable that the current installation is invalid so opam dune-lint could refuse to run the plugin without reinstallation?

Thoughts, @AltGr? It will affect any plugin which uses the opam libraries which, ideally, should be all of them!

@dra27 dra27 added this to the 2.1.0~rc milestone Mar 30, 2021
@dra27
Copy link
Member Author

dra27 commented Mar 30, 2021

(NB - this should resolved before the release candidate but that resolution may be "won't fix")

@AltGr
Copy link
Member

AltGr commented Mar 30, 2021

Hmm, this feels to me that the problem comes from linking to opam-state rather than from being a plugin ; the two are of course linked, and this still means that at least half of the problem is on opam's side; but the proposed fix might be the wrong way around. There may be plugins that remain valid...

Handling this correctly I think would mean removing or recompiling the plugin package itself -- and not just the link -- during format upgrade... which would be quite involved and also mean many more things could go wrong during format upgrades :/ so not much better.

Another workaround could be to run a check on opam foo, verifying that foo is actually current in the switch it was installed from (and proposing to upgrade/reinstall if not, like for the initial install). But that means loading more state than we normally need.

Maybe it's not that big a problem that opam dune-lint fails after an upgrade, if we manage to have it print a sensible hint as to how to solve the problem (which may be done from opam-state) ?

@dra27
Copy link
Member Author

dra27 commented Mar 30, 2021

It seems better to have some plugins unnecessarily rebuilt after a format upgrade than have some which did need rebuilding just be broken? We can't fix it with an opam-state message - the fix needs to be in the old version of the library rather than the new, sadly (the proverbial cat is out of the bag!)

We could cache the validity of packages in the switch state to save loading the entire universe state?

@talex5
Copy link
Contributor

talex5 commented Mar 31, 2021

We can't fix it with an opam-state message - the fix needs to be in the old version of the library rather than the new, sadly (the proverbial cat is out of the bag!)

We've got to update opam-dune-lint anyway, so we can just require opam-state >= whatever version has the check.

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 a pull request may close this issue.

3 participants