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.

Sign up for GitHub

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

Jump to bottom

Reinstall plugin package if the symlink is missing #4621

Merged
merged 2 commits into from
Apr 16, 2021
Merged

Reinstall plugin package if the symlink is missing #4621

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8899856
Introduce OpamPath.plugin_prefix
dra27 Mar 31, 2021
2c19103
Reinstall plugin if symlink missing
dra27 Mar 31, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions master_changes.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ New option/command/subcommand are prefixed with ◈.
* Add cli versioning for opam environment variables [#4606 @rjbou]
* Deprecated `build-doc`, `build-test`, `make` [#4581 @rjbou]
* Add cli versioning for enums of flags with predefined enums [#4606 @rjbou]
* Ensure the symlink for a plugin is maintained on each invocation [#4621 @dra27 - partially fixes #4619]

## Init
* Introduce a `default-invariant` config field, restore the 2.0 semantics for
Expand Down
36 changes: 24 additions & 12 deletions src/client/opamCliMain.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,6 @@ let rec preprocess_argv cli yes args =

(* Handle git-like plugins *)
let check_and_run_external_commands () =
let plugin_prefix = "opam-" in
(* Pre-process the --yes and --cli options *)
let (cli, yes, argv) =
match Array.to_list Sys.argv with
Expand Down Expand Up @@ -132,13 +131,16 @@ let check_and_run_external_commands () =
then (cli, argv)
else
(* No such command, check if there is a matching plugin *)
let command = plugin_prefix ^ name in
let command = OpamPath.plugin_prefix ^ name in
let answer = if yes then Some true else OpamCoreConfig.E.yes () in
OpamCoreConfig.init ~answer ();
OpamFormatConfig.init ();
let root_dir = OpamStateConfig.opamroot () in
let has_init = OpamStateConfig.load_defaults root_dir <> None in
let plugins_bin = OpamPath.plugins_bin root_dir in
let plugin_symlink_present =
OpamFilename.is_symlink (OpamPath.plugin_bin root_dir (OpamPackage.Name.of_string name))
in
let env =
if has_init then
let updates =
Expand All @@ -155,18 +157,18 @@ let check_and_run_external_commands () =
Unix.environment ()
in
match OpamSystem.resolve_command ~env command with
| Some command ->
| Some command when plugin_symlink_present ->
let argv = Array.of_list (command :: args) in
raise (OpamStd.Sys.Exec (command, argv, env))
| None when not has_init -> (cli, argv)
| None ->
| cmd ->
(* Look for a corresponding package *)
match OpamStateConfig.get_switch_opt () with
| None -> (cli, argv)
| Some sw ->
OpamGlobalState.with_ `Lock_none @@ fun gt ->
OpamSwitchState.with_ `Lock_none gt ~switch:sw @@ fun st ->
let prefixed_name = plugin_prefix ^ name in
let prefixed_name = OpamPath.plugin_prefix ^ name in
let candidates =
OpamPackage.packages_of_names
(Lazy.force st.available_packages)
Expand All @@ -184,7 +186,7 @@ let check_and_run_external_commands () =
in
let installed = OpamPackage.Set.inter plugins st.installed in
if OpamPackage.Set.is_empty candidates then (cli, argv)
else if not OpamPackage.Set.(is_empty installed) then
else if not OpamPackage.Set.(is_empty installed) && cmd = None then
(OpamConsole.error
"Plugin %s is already installed, but no %s command was found.\n\
Try upgrading, and report to the package maintainer if \
Expand All @@ -206,18 +208,28 @@ let check_and_run_external_commands () =
then
let nv =
try
OpamPackage.max_version plugins
(OpamPackage.Name.of_string prefixed_name)
(* If the command was resolved, attempt to find the package to reinstall. *)
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)

with Not_found ->
OpamPackage.max_version plugins
(OpamPackage.Name.of_string name)
try
OpamPackage.max_version plugins
(OpamPackage.Name.of_string prefixed_name)
with Not_found ->
OpamPackage.max_version plugins
(OpamPackage.Name.of_string name)
in
OpamRepositoryConfig.init ();
OpamSolverConfig.init ();
OpamClientConfig.init ();
OpamSwitchState.with_ `Lock_write gt (fun st ->
OpamSwitchState.drop @@
OpamClient.install st [OpamSolution.eq_atom_of_package nv]
OpamSwitchState.drop @@ (
if cmd = None then
OpamClient.install st [OpamSolution.eq_atom_of_package nv]
else
OpamClient.reinstall st [OpamSolution.eq_atom_of_package nv])
);
match OpamSystem.resolve_command ~env command with
| None ->
Expand Down
6 changes: 4 additions & 2 deletions src/format/opamPath.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,15 +57,17 @@ let backup_dir t = t / "backup"

let backup t = backup_dir t /- backup_file ()

let plugin_prefix = "opam-"

let plugins t = t / "plugins"

let plugins_bin t = plugins t / "bin"

let plugin_bin t name =
let sname = OpamPackage.Name.to_string name in
let basename =
if OpamStd.String.starts_with ~prefix:"opam-" sname then sname
else "opam-" ^ sname
if OpamStd.String.starts_with ~prefix:plugin_prefix sname then sname
else plugin_prefix ^ sname
in
plugins_bin t // basename

Expand Down
3 changes: 3 additions & 0 deletions src/format/opamPath.mli
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,9 @@ val backup_dir: t -> dirname
(** Backup file for state export *)
val backup: t -> switch_selections OpamFile.t

(** The prefix for plugin commands (["opam-"]) *)
val plugin_prefix : string

(** The directory for plugins data {i $opam/plugins} *)
val plugins: t -> dirname

Expand Down
2 changes: 1 addition & 1 deletion src/state/opamFileTools.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -498,7 +498,7 @@ let t_lint ?check_extra_files ?(check_upstream=false) ?(all=false) t =
match t.OpamFile.OPAM.name with
| None -> false
| Some name ->
not (OpamStd.String.starts_with ~prefix:"opam-"
not (OpamStd.String.starts_with ~prefix:OpamPath.plugin_prefix
(OpamPackage.Name.to_string name)));
(let unclosed =
List.fold_left (fun acc s ->
Expand Down