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

Incorrect removal of a package with conditional dependency #4727

Closed
rjbou opened this issue Jun 23, 2021 · 3 comments · Fixed by #4995
Closed

Incorrect removal of a package with conditional dependency #4727

rjbou opened this issue Jun 23, 2021 · 3 comments · Fixed by #4995
Assignees
Projects
Milestone

Comments

@rjbou
Copy link
Collaborator

rjbou commented Jun 23, 2021

If you have a switch with dune & ocaml < 4.08, you automatically have ocamlfind-secondary installed, but if you upgrade your ocaml, you can't remove ocamlfind-secondary without removing dune.
dune.2.8.5 have:

depends      ("ocaml" {>= "4.08"} | ("ocaml" {< "4.08~~"} & "ocamlfind-secondary"))

Calculation of reverse dependencies for automatic removal is too wide, it includes packages from rev deps disjunctions.

@rjbou rjbou added this to the 2.2.0~alpha milestone Jun 23, 2021
@rjbou rjbou added this to To do in Opam 2.2.0 via automation Jun 23, 2021
@AltGr
Copy link
Member

AltGr commented Jun 25, 2021

The incriminated reverse dependency calculation is here:

opam/src/client/opamClient.ml

Lines 1374 to 1377 in 82fb1ad

let to_remove =
OpamSolver.reverse_dependencies ~build:true ~post:true
~depopts:false ~installed:true universe packages
in

This is useful when using the "autoremove" flag.

We could try to have a separate computation for "guaranteed" reverse dependencies (rather than the "inclusive" set now computed) but that would be quite involved and trigger other weirdnesses. The proper solution would probably be, instead, to remove this preprocessing and let the solver do the work: define an internal Cudf package flag "manually-installed" and adjust the solver criteria to something in the lines of -count[changed,manually-installed],+removed,...

@rjbou rjbou moved this from To do to To do: opam file updates in Opam 2.2.0 Nov 16, 2021
@rjbou rjbou moved this from To do: opam file updates to To do in Opam 2.2.0 Nov 16, 2021
@rjbou
Copy link
Collaborator Author

rjbou commented Dec 17, 2021

@rjbou add a test

@dra27 dra27 moved this from To do to In progress in Opam 2.2.0 Jan 14, 2022
@AltGr
Copy link
Member

AltGr commented Jan 14, 2022

From a quick test:

  • on master, this is solved in the case of a simple opam remove:
    % opam remove --show ocamlfind-secondary
    The following actions would be faked:
      ⊘ remove    ocamlfind-secondary 1.9.1
      ↻ recompile dune                2.9.1 [uses ocamlfind-secondary]
    ===== ↻ 1   ⊘ 1 =====
    
  • but remains with remove -a:
    % opam remove ocamlfind-secondary --show -a
    The following actions would be faked:
      ⊘ remove dune                     2.9.1    [uses ocamlfind-secondary]
      ⊘ remove ocamlfind-secondary      1.9.1
      ⊘ remove ocamlfind                1.9.1
      ⊘ remove ocaml-secondary-compiler 4.08.1-1
    ===== ⊘ 4 =====
    
    dune was in my case part of the "root" packages and shouldn't have been removed.

AltGr added a commit to AltGr/opam that referenced this issue Jan 14, 2022
AltGr added a commit to AltGr/opam that referenced this issue Jan 17, 2022
AltGr added a commit to AltGr/opam that referenced this issue Jan 17, 2022
kit-ty-kate pushed a commit to AltGr/opam that referenced this issue Jan 21, 2022
rjbou pushed a commit to AltGr/opam that referenced this issue Jan 21, 2022
Opam 2.2.0 automation moved this from In progress to Done Jan 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Opam 2.2.0
  
Done
Development

Successfully merging a pull request may close this issue.

2 participants