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

Reinstall plugin package if the symlink is missing #4621

Merged
merged 2 commits into from
Apr 16, 2021

Conversation

dra27
Copy link
Member

@dra27 dra27 commented Mar 31, 2021

This is a partial fix for #4619 (the other part would be to remove the plugin symlinks at format upgrade).

This tweaks the plugin detection slightly so that if the symlink is missing, the command is still resolved, but package will be reinstalled in the current switch. This will recreate the symlink and also apply any changes to dependencies (e.g. after an opam client upgrade).

It's demonstrated by this slightly convoluted Dockerfile - commenting out the final rm command shows the existing error.

FROM ocaml/opam:debian-ocaml-4.12
WORKDIR /src
RUN sudo chown opam /src
RUN git clone https://github.com/dra27/opam.git
WORKDIR /src/opam
RUN opam pin add -n dose3 5.0.1-1
RUN git checkout reinstall-plugins ; opam install dose3 opam-file-format mccs cmdliner
RUN opam exec -- ./configure && opam exec -- make
WORKDIR /src/test
RUN sudo chown opam /src/test
RUN opam install dune
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#bf451c8cf2d4f2be3c7d979b613c79588b55a172'
RUN opam dune-lint
#RUN sudo rm /usr/bin/opam && sudo ln /usr/bin/opam-2.1 /usr/bin/opam
RUN sudo rm /usr/bin/opam && sudo cp /src/opam/opam /usr/bin/opam
RUN opam update
RUN rm ~/.opam/plugins/bin/*
RUN opam dune-lint

Copy link
Member

@AltGr AltGr left a comment

Choose a reason for hiding this comment

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

Nice!

if cmd = None then
raise Not_found
else
OpamPackage.package_of_name installed (OpamPackage.Name.of_string prefixed_name)
Copy link
Member

Choose a reason for hiding this comment

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

Won't this cause problems if the currently installed version is no longer available ? This could happen e.g. with available: depending on opam-version after an opam root upgrade, where an upgrade of the plugin becomes mandatory, and cause an error during resolution, requiring a manual opam upgrade.
I don't remember why this code used to force the latest version (within availability) though

Copy link
Member Author

Choose a reason for hiding this comment

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

It turns out it doesn't, but I think that's a bug - the availability check is not done for opam reinstall!

The alternative would be to upgrade the package by name instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't remember why this code used to force the latest version (within availability) though

OPAM 1.x days? (easing or even avoiding the solver?)

Copy link
Collaborator

Choose a reason for hiding this comment

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

The candidates package already have the availability check, so if a plugin is no more available it won't be in installed and its latest version will be "newly" installed:

$ opam install opam-depext.1.1.3 #without opam < 2.1 condition
$ opam update # reintroduce opam < 2.1 condition
$ rm <link>
$ opam depext
Opam plugin "depext" is not installed. Install it on the current switch? [Y/n] y
opam-depext.1.1.5 is not installed. Install it? [Y/n] y
The following actions will be performed:
  ↗ upgrade opam-depext 1.1.3 to 1.1.5

<><> Processing actions <><><><><><><><><><><><><><><><><><><><><><><><><><><><>
⬇ retrieved opam-depext.1.1.5  (cached)
⊘ removed   opam-depext.1.1.3
∗ installed opam-depext.1.1.5
Done.

On the availability check for reinstall, this (specific) case is handled by the orphans mechanism, and can't be reinstalled:

$ opam install opam-depext.1.1.3 #without opam < 2.1 condition
$ opam update # reintroduce opam < 2.1 condition
$ opam reinstall opam-depext
[ERROR] Sorry, these packages are no longer available from the repositories: opam-depext (= 1.1.3)

@rjbou rjbou added this to the 2.1.0~rc milestone Apr 9, 2021
@dra27 dra27 force-pushed the reinstall-plugins branch from 8818ed2 to b60c8dc Compare April 13, 2021 20:06
@dra27
Copy link
Member Author

dra27 commented Apr 14, 2021

Need to double-check the reinstall question above, but otherwise this is good to go.

@rjbou
Copy link
Collaborator

rjbou commented Apr 15, 2021

A rebase and good to merge for me!

@rjbou rjbou force-pushed the reinstall-plugins branch from b60c8dc to 8633e78 Compare April 15, 2021 10:11
dra27 added 2 commits April 15, 2021 15:07
At present opam plugin will simply run opam-plugin if it is resolved in
the environment of the current switch.

Now $OPAMROOT/plugins/bin is checked for the required symlink. The
command is still resolved as normal, but the presence or not of the
symlink is used to control whether opam _installs_ the plugin package or
does a reinstall.

The reinstall both recreates the symlink and ensures that any changes to
the plugin have been picked up.
@rjbou rjbou force-pushed the reinstall-plugins branch from 8633e78 to 2c19103 Compare April 15, 2021 13:07
@dra27
Copy link
Member Author

dra27 commented Apr 16, 2021

I agree - the reinstall question is already fixed - IIRC I tested on an old opam!

@dra27 dra27 merged commit 477d9d1 into ocaml:master Apr 16, 2021
@dra27 dra27 deleted the reinstall-plugins branch April 16, 2021 08:09
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