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

opam-dune-lint gets the wrong package for some dependencies #53

Closed
talex5 opened this issue Oct 23, 2023 · 4 comments
Closed

opam-dune-lint gets the wrong package for some dependencies #53

talex5 opened this issue Oct 23, 2023 · 4 comments
Assignees
Labels
bug Something isn't working

Comments

@talex5
Copy link
Contributor

talex5 commented Oct 23, 2023

e.g. with Eio, ocaml-ci is currently reporting:

eio_main.opam: changes needed:
  "fmt" {>= "0.9.0"}                       [from lib_eio_linux, lib_eio_posix, lib_eio_windows]
  "iomux" {>= "0"}                         [from lib_eio_posix]
  "logs" {>= "0.7.0"}                      [from lib_eio_linux]
  "uring" {>= "0"}                         [from lib_eio_linux]

But none of these directories is (directly) part of the eio_main package anyway.

For example, iomux is a dependency only of the eio_posix library, which is part of the eio_posix package (see https://github.com/ocaml-multicore/eio/blob/60e67472e3726192c4f6b9fbff81db5eb15b8a27/lib_eio_posix/dune#L10).

If I add (package eio_posix) to this, dune complains:

Error: This library has a public_name, it already belongs to the package eio_posix

And dune describe says:

  (library
   ((names (eio_posix))
    (extensions ())
    (package (eio_posix))
    (source_dir lib_eio_posix)
    (external_deps
     ((fmt required)
      (iomux required)))
    (internal_deps
     ((eio required)
      (eio.utils required)
      (eio.unix required)))))

So dune is reporting that iomux is part of eio_posix.

@moyodiallo moyodiallo self-assigned this Oct 23, 2023
@moyodiallo moyodiallo added the bug Something isn't working label Oct 23, 2023
@moyodiallo
Copy link
Collaborator

This is a bug, by default opam-dune-lint also resolve the transitive dependencies because a public library could depends on a private library. It doesn't resolve the internal_deps which are public.

dune describe doesn't printing lib_eio_linux, lib_eio_posix, lib_eio_windows as internal_deps, because of the alternative select dune stanza.

(library
  (name eio_main)
  (public_name eio_main)
  (libraries
    (select linux_backend.ml from
            (eio_linux -> linux_backend.enabled.ml)
            (          -> linux_backend.disabled.ml))
    (select posix_backend.ml from
            (eio_posix -> posix_backend.enabled.ml)
            (          -> posix_backend.disabled.ml))
    (select windows_backend.ml from
            (eio_windows -> windows_backend.enabled.ml)
            (            -> windows_backend.disabled.ml))
    ))

This is why we're seeing this bug. It could be easily solved by sharing a list of all the public libraries.

moyodiallo added a commit to moyodiallo/opam-dune-lint that referenced this issue Oct 24, 2023
This is a fix for the issue ocurrent#53. The public optional libraries were
resolved.
moyodiallo added a commit to moyodiallo/opam-dune-lint that referenced this issue Oct 24, 2023
This is a fix for the issue ocurrent#53. The public optional libraries were
resolved.
moyodiallo added a commit to moyodiallo/opam-dune-lint that referenced this issue Oct 24, 2023
This is a fix for the issue ocurrent#53. The public optional libraries were
resolved.
@moyodiallo
Copy link
Collaborator

moyodiallo commented Oct 24, 2023

dune describe doesn't printing lib_eio_linux, lib_eio_posix, lib_eio_windows as internal_deps, because of the alternative select dune stanza.

the command dune describe external-lib-deps gives:

(library
  ((names (eio_main))
   (extensions ())
   (package (eio_main))
   (source_dir lib_main)
   (external_deps ())
   (internal_deps
    ((eio_linux optional)
     (eio_posix optional)
     (eio_windows optional)))))

My bad, my description above had some mistake. The internal optional public libs were resolve this is why the bug. Fixed here #54.

moyodiallo added a commit to moyodiallo/opam-dune-lint that referenced this issue Oct 24, 2023
This is a fix for the issue ocurrent#53. The public optional libraries were
resolved.
@moyodiallo
Copy link
Collaborator

The fix is deployed on ocaml-ci, is it OK to close this issue ? @talex5

@talex5
Copy link
Contributor Author

talex5 commented Oct 25, 2023

That seems to work - thanks for the quick fix!

(it's now saying WARNING: can't find opam package providing "runtime_events", but that's another problem!)

@talex5 talex5 closed this as completed Oct 25, 2023
moyodiallo added a commit to moyodiallo/opam-repository that referenced this issue Oct 31, 2023
CHANGES:

- Fix the issue ocurrent/opam-dune-lint#53. Skip resolving a public library when it is added as optional dependency(dune's libraries stanza) (@moyodiallo ocurrent/opam-dune-lint#54).

- Print all the errors before the exit (@moyodiallo ocurrent/opam-dune-lint#55).
nberth pushed a commit to nberth/opam-repository that referenced this issue Jun 18, 2024
CHANGES:

- Fix the issue ocurrent/opam-dune-lint#53. Skip resolving a public library when it is added as optional dependency(dune's libraries stanza) (@moyodiallo ocurrent/opam-dune-lint#54).

- Print all the errors before the exit (@moyodiallo ocurrent/opam-dune-lint#55).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants