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

Make sure that recursive dependencies are not reflexive #4804

Closed

Conversation

LasseBlaauwbroek
Copy link
Contributor

opam list --recursive --recursive --required-by=package is reflexive (it shows the package itself). This does not seem correct, since a package does not require itself. Note that without --recursive, the command is not reflexive.

@kit-ty-kate
Copy link
Member

Thanks! fixes #4446

@kit-ty-kate
Copy link
Member

kit-ty-kate commented Aug 14, 2021

The tests fail and it might be related. Could you have a look at them? e.g.

diff --git a/tests/reftests/legacy-git.test b/tests/reftests/legacy-git.out
index a629f71..63d38d2 100644
--- a/tests/reftests/legacy-git.test
+++ b/tests/reftests/legacy-git.out
@@ -747,20 +747,16 @@ Done.
 ocaml.system
 P1.1
 ### opam remove P1
-The following actions will be performed:
-  - remove P1 1
-
-<><> Processing actions <><><><><><><><><><><><><><><><><><><><><><><><><><><><>
--> removed   P1.1
-Done.
+Nothing to do.
 ### opam list -is --columns=package
 ocaml.system
+P1.1
 ### : LIST :
 ### opam list -A
 # Packages matching: any
 # Name # Installed # Synopsis
 ocaml  system
-P1     --          A very useful package
+P1     1           A very useful package
 P2     --          An other very useful package
 P3     --          Testing version names
 P4     --          Testing transitive closure

@LasseBlaauwbroek
Copy link
Contributor Author

My guess is that some functions rely on the reflexive behavior (for example, in order to calculate package removal). I changed it so that the non-reflexive behavior only occurs in the list command.

@kit-ty-kate kit-ty-kate added this to the 2.2.0~alpha milestone Aug 14, 2021
@AltGr
Copy link
Member

AltGr commented Aug 20, 2021

Thanks! Indeed this was inconsistent.

A note of caution however: this is incorrect with multiple packages, e.g. --required-by=a,b should still return a if b depends on a.

@LasseBlaauwbroek
Copy link
Contributor Author

I guess this is true, although it depends a bit on how one reads the documentation of --required-by and --depends-on.

The only way I can think of fixing this is to run a separate query for each of the requested atoms and then take the union of the results. Would that be acceptable, or is that too slow?

@AltGr
Copy link
Member

AltGr commented Aug 24, 2021

I guess this is true, although it depends a bit on how one reads the documentation of --required-by and --depends-on.

Hm, that doc probably needs some effort then. The idea is that --depends foo,bar is not the same as --depends foo --depends bar, because the latter wouldn't need an extra syntax.

It's not a very common pattern, so I guess the solution you propose would be fine, dependency queries are not that expensive; and I can't think of an other one :)

@kit-ty-kate
Copy link
Member

Superseded by #5075 which supports the --required-by=a,b case.
Thanks a lot for getting the ball rolling!

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.

opam list --recursive --depends-on <pkg> should not include the package itself
3 participants