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

Remove dependency cycles #18124

Closed
wants to merge 2 commits into from
Closed

Remove dependency cycles #18124

wants to merge 2 commits into from

Conversation

samoht
Copy link
Member

@samoht samoht commented Feb 10, 2021

No description provided.

@samoht
Copy link
Member Author

samoht commented Feb 10, 2021

This makes opam admin check --cycle --ignore-test-doc return without error.

@samoht
Copy link
Member Author

samoht commented Feb 10, 2021

It's probably not worth testing the revdpes on the entiere universe. @dra27 and @kit-ty-kate are you ok to merge this?

@camelus
Copy link
Contributor

camelus commented Feb 10, 2021

Commit: 019b3aa

A pull request by opam-seasoned @samoht.

🌤️ opam-lint warnings 019b3aa
  • ocamlfind-secondary.1.8.1 has some warnings:

    • warning 37: Missing field 'dev-repo'
    • warning 47: Synopsis (or description first line) should start with a capital and not end with a dot
  • These packages passed lint tests: dune-configurator.2.7.1, dune-configurator.2.8.0, dune-configurator.2.8.1, dune-configurator.2.8.2, fileutils.0.6.0, graphics.5.0.0


☀️ Installability check (+0)

@@ -10,6 +10,7 @@ depends: [
"ocaml"
"jbuilder" {>= "1.0+beta11"}
]
conflicts:["dune-configurator"]
Copy link
Member

Choose a reason for hiding this comment

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

What was the issue here? I'd rather have the constraint in the correct file (dune-configurator) rather than here. It's much easier to understand and maintain this way.

@@ -22,6 +22,7 @@ depends: [
"conf-pkg-config" {os != "win32"}
"ocaml" {>= "4.09.0~~"}
]
conflicts: ["ocamlfind-secondary"]
Copy link
Member

Choose a reason for hiding this comment

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

same question here. Something tells me the constraint should be in ocamlfind-secondary.

@samoht
Copy link
Member Author

samoht commented Feb 10, 2021

@kit-ty-kate there is a cycle between these packages which confuses the solver:

dune (>= 2.0.0 & < 2.1.1) -> ocamlfind-secondary = 1.8.1 -> ocamlfind = 1.8.1 -> graphics = 5.0.0

This PR breaks that cycle explicitly by adding a conflict in the middle of the dependency chain. That same conflict can be added either in dune or ocamlfind or grapics but not in ocamlfind-secondary as it's not part of the chain. I can add a comment in the file to make that clearer if you think that's useful.

@samoht
Copy link
Member Author

samoht commented Feb 11, 2021

@kit-ty-kate I've fixed the location of the graphics constraint ; for the result one it's easier to patch the old result.1.3. than all the existing (and future) release of dune-configurator. Is that ok with you?

@samoht
Copy link
Member Author

samoht commented Feb 11, 2021

The revdep testing are killing the CI. @kit-ty-kate is this ok if I merge this quickly?

@kit-ty-kate
Copy link
Member

This PR might not be necessary as opam admin check --cycle might not take coinstallability into account according to @AltGr. I suggest to not merge until we know more.
Opam issue: ocaml/opam#4541

@kit-ty-kate kit-ty-kate marked this pull request as draft February 11, 2021 22:56
@samoht
Copy link
Member Author

samoht commented Feb 23, 2021

Closing as it needs caml/opam#4541 to be resolved first.

@samoht samoht closed this Feb 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants