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

Simplify error message when deps don't build with dune #384

Merged

Conversation

gridbugs
Copy link
Collaborator

When trying to generate a lockfile on a package which has dependencies which don't use dune as their build system, the diagnostic message generated by opam-0install is of little use. This change removes the opam-0install diagnostic message from the output of opam monorepo lock in the case where it fails due to dependencies not building with dune.

Additionally this rewords the error message to highlight the salient information. In particular the list of non-dune packages is printed as a bulleted list rather than a comma-separated list inside the error message (it often takes me a minute to find the list of packages in the old error message).

@gridbugs gridbugs changed the title opaSimplify error message when deps don't build with dune Simplify error message when deps don't build with dune Apr 27, 2023
@gridbugs
Copy link
Collaborator Author

An example output of opam monorepo lock using this opam file:

opam-version: "2.0"
depends: [
  "cmdliner"
]

Before this change:

opam_monorepo.exe: [WARNING] Couldn't calculate a set of packages to satisfy the request. Note that opam monorepo lock will fail if not all of the project dependencies use dune as their build system, in your project that would be cmdliner. To solve this issue there exists a dune-universe opam-repository which contains dune ports for some opam packages, but it is currently not set in your current switch. If you wish to set it up, run the following command:
opam repository add dune-universe git+https://github.com/dune-universe/opam-overlays.git
opam_monorepo.exe: [ERROR] Can't find all required versions.
Selected: base-bigarray.base base-domains.base base-nnp.base
          base-threads.base base-unix.base ocaml-config.3
          ocaml-options-vanilla.1 package.zdev ocaml-base-compiler&package
          ocaml-base-compiler ocaml-base-compiler ocaml base-domains
          ocaml-base-compiler
- cmdliner -> (problem)
    No usable implementations:
      cmdliner.1.2.0: Doesn't build with dune
      cmdliner.1.1.1: Doesn't build with dune
      cmdliner.1.1.0: Doesn't build with dune
      cmdliner.1.0.4: Doesn't build with dune
      cmdliner.1.0.3: Doesn't build with dune
      ...
- ocaml -> ocaml.5.0.0
    ocaml-base-compiler 5.0.0 requires = 5.0.0
- ocaml-base-compiler -> ocaml-base-compiler.5.0.0
    ocaml-base-compiler|ocaml-variants|ocaml-system ocaml-base-compiler requires >= 5.0.0~ & < 5.0.1~

After this change:

opam_monorepo.exe: [ERROR] Some dependencies cannot be built with dune!

opam-monorepo requires that all dependencies use dune as their build system.

These dependencies (possibly transitive) don't use dune as their build system:
- cmdliner

The dune-universe opam repository (git+https://github.com/dune-universe/opam-overlays.git) contains dune ports of some popular packages to help build more packages with dune. It doesn't appear to be set up on this switch. Adding it to this switch may fix this issue. Add the dune-universe opam repository to this switch by running the command:

opam repository add dune-universe git+https://github.com/dune-universe/opam-overlays.git

@gridbugs gridbugs force-pushed the no-diagnostic-when-deps-dont-build-with-dune branch from b3b06be to eefbf05 Compare April 27, 2023 06:56
Copy link
Member

@Leonidas-from-XIV Leonidas-from-XIV left a comment

Choose a reason for hiding this comment

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

Looks like a pretty solid improvement, thanks for the contribution!

CHANGES.md Show resolved Hide resolved
cli/lock.ml Outdated Show resolved Hide resolved
cli/lock.ml Outdated Show resolved Hide resolved
cli/lock.ml Outdated Show resolved Hide resolved
@Leonidas-from-XIV
Copy link
Member

Ehh, I see there's a CI failure. That needs to be updated, but it looks like it is just about the exact output of the command, not a fundamental property of the PR that it breaks something.

When trying to generate a lockfile on a package which has dependencies
which don't use dune as their build system, the diagnostic message
generated by opam-0install is of little use. This change removes the
opam-0install diagnostic message from the output of `opam monorepo lock`
in the case where it fails due to dependencies not building with dune.

Additionally this rewords the error message to highlight the salient
information. In particular the list of non-dune packages is printed as a
bulleted list rather than a comma-separated list inside the error
message (it often takes me a minute to find the list of packages in the
old error message).
@gridbugs gridbugs force-pushed the no-diagnostic-when-deps-dont-build-with-dune branch from eefbf05 to 250714d Compare April 28, 2023 01:23
@gridbugs gridbugs merged commit 46b287f into tarides:main Apr 28, 2023
Leonidas-from-XIV added a commit to Leonidas-from-XIV/opam-repository that referenced this pull request May 3, 2023
CHANGES:

### Added

- Display warning when a package to be locked is missing a `dev-repo` field and
  is being skipped because of it (tarides/opam-monorepo#341, tarides/opam-monorepo#362, @kit-ty-kate, @Leonidas-from-XIV)
- Add option `--keep-symlinked-dir` to preserve symlinks in `duniverse/`, which
  can be useful for local development. (tarides/opam-monorepo#348, tarides/opam-monorepo#366, @hannesm,
  @Leonidas-from-XIV)

### Changed

- Canonicalize the URLs of the OPAM `dev-repo` fields to be able to detect more
  semantically equivalent URLs, this should reduce the risk of build failures
  due to duplicate code pulled (tarides/opam-monorepo#118, tarides/opam-monorepo#365 @TheLortex, @Leonidas-from-XIV)

- Simple the error message printed when dependencies don't use dune as their
  build system. The opam-0install diagnostic message is no longer printed in
  this case and the message has been reformatted and reworded to make the
  salient information easier to see. (tarides/opam-monorepo#384, @gridbugs)

### Fixed

- Error in case where multiple packages with different dev-repo fields would be
  placed in the same duniverse directory (tarides/opam-monorepo#377, @gridbugs)

- Fix a failure when using opam-monorepo with an opam 2.2 root
  (tarides/opam-monorepo#379, @kit-ty-kate)

- Fix assertion failure when prefix of "lock" subcommand is used (tarides/opam-monorepo#381,
  @gridbugs)

- Treat packages without build commands as virtual only if also lack install
  commands, as some non-virtual packages might only have install commands.
  (tarides/opam-monorepo#376 @Leonidas-from-XIV, @gridbugs)
Leonidas-from-XIV added a commit to Leonidas-from-XIV/opam-repository that referenced this pull request Sep 11, 2023
CHANGES:

### Added

- Display warning when a package to be locked is missing a `dev-repo` field and
  is being skipped because of it (tarides/opam-monorepo#341, tarides/opam-monorepo#362, @kit-ty-kate, @Leonidas-from-XIV)
- Add option `--keep-symlinked-dir` to preserve symlinks in `duniverse/`, which
  can be useful for local development. (tarides/opam-monorepo#348, tarides/opam-monorepo#366, @hannesm,
  @Leonidas-from-XIV)
- Adopt the OCaml Code of Conduct (tarides/opam-monorepo#391, @rikusilvola)
- Add solver tests (tarides/opam-monorepo#394, @samoht)

### Changed

- Canonicalize the URLs of the OPAM `dev-repo` fields to be able to detect more
  semantically equivalent URLs, this should reduce the risk of build failures
  due to duplicate code pulled (tarides/opam-monorepo#118, tarides/opam-monorepo#365 @TheLortex, @Leonidas-from-XIV)

- Simple the error message printed when dependencies don't use dune as their
  build system. The opam-0install diagnostic message is no longer printed in
  this case and the message has been reformatted and reworded to make the
  salient information easier to see. (tarides/opam-monorepo#384, @gridbugs)

- Encode `dev-repo` constraints in the opam solver - this allows to resolve
  more involved version constraints that were failing before (tarides/opam-monorepo#396, @samoht)

### Deprecated

### Fixed

- Error in case where multiple packages with different dev-repo fields would be
  placed in the same duniverse directory (tarides/opam-monorepo#377, @gridbugs)

- Fix a failure when using opam-monorepo with an opam 2.2 root
  (tarides/opam-monorepo#379, @kit-ty-kate)

- Fix assertion failure when prefix of "lock" subcommand is used (tarides/opam-monorepo#381,
  @gridbugs)

- Treat packages without build commands as virtual only if also lack install
  commands, as some non-virtual packages might only have install commands.
  (tarides/opam-monorepo#376 @Leonidas-from-XIV, @gridbugs)

- Improve the ordering of package candidates by putting broken packages at
  then end of the list (tarides/opam-monorepo#395, tarides/opam-monorepo#397, @samoht)

### Removed

### Security
Leonidas-from-XIV added a commit to Leonidas-from-XIV/opam-repository that referenced this pull request Sep 11, 2023
CHANGES:

### Added

- Display warning when a package to be locked is missing a `dev-repo` field and
  is being skipped because of it (tarides/opam-monorepo#341, tarides/opam-monorepo#362, @kit-ty-kate, @Leonidas-from-XIV)
- Add option `--keep-symlinked-dir` to preserve symlinks in `duniverse/`, which
  can be useful for local development. (tarides/opam-monorepo#348, tarides/opam-monorepo#366, @hannesm,
  @Leonidas-from-XIV)
- Adopt the OCaml Code of Conduct (tarides/opam-monorepo#391, @rikusilvola)
- Add solver tests (tarides/opam-monorepo#394, @samoht)

### Changed

- Canonicalize the URLs of the OPAM `dev-repo` fields to be able to detect more
  semantically equivalent URLs, this should reduce the risk of build failures
  due to duplicate code pulled (tarides/opam-monorepo#118, tarides/opam-monorepo#365 @TheLortex, @Leonidas-from-XIV)

- Simple the error message printed when dependencies don't use dune as their
  build system. The opam-0install diagnostic message is no longer printed in
  this case and the message has been reformatted and reworded to make the
  salient information easier to see. (tarides/opam-monorepo#384, @gridbugs)

- Encode `dev-repo` constraints in the opam solver - this allows to resolve
  more involved version constraints that were failing before (tarides/opam-monorepo#396, @samoht)

### Deprecated

### Fixed

- Error in case where multiple packages with different dev-repo fields would be
  placed in the same duniverse directory (tarides/opam-monorepo#377, @gridbugs)

- Fix a failure when using opam-monorepo with an opam 2.2 root
  (tarides/opam-monorepo#379, @kit-ty-kate)

- Fix assertion failure when prefix of "lock" subcommand is used (tarides/opam-monorepo#381,
  @gridbugs)

- Treat packages without build commands as virtual only if also lack install
  commands, as some non-virtual packages might only have install commands.
  (tarides/opam-monorepo#376 @Leonidas-from-XIV, @gridbugs)

- Improve the ordering of package candidates by putting broken packages at
  then end of the list (tarides/opam-monorepo#395, tarides/opam-monorepo#397, @samoht)

### Removed

### Security
nberth pushed a commit to nberth/opam-repository that referenced this pull request Jun 18, 2024
CHANGES:

### Added

- Display warning when a package to be locked is missing a `dev-repo` field and
  is being skipped because of it (tarides/opam-monorepo#341, tarides/opam-monorepo#362, @kit-ty-kate, @Leonidas-from-XIV)
- Add option `--keep-symlinked-dir` to preserve symlinks in `duniverse/`, which
  can be useful for local development. (tarides/opam-monorepo#348, tarides/opam-monorepo#366, @hannesm,
  @Leonidas-from-XIV)
- Adopt the OCaml Code of Conduct (tarides/opam-monorepo#391, @rikusilvola)
- Add solver tests (tarides/opam-monorepo#394, @samoht)

### Changed

- Canonicalize the URLs of the OPAM `dev-repo` fields to be able to detect more
  semantically equivalent URLs, this should reduce the risk of build failures
  due to duplicate code pulled (tarides/opam-monorepo#118, tarides/opam-monorepo#365 @TheLortex, @Leonidas-from-XIV)

- Simple the error message printed when dependencies don't use dune as their
  build system. The opam-0install diagnostic message is no longer printed in
  this case and the message has been reformatted and reworded to make the
  salient information easier to see. (tarides/opam-monorepo#384, @gridbugs)

- Encode `dev-repo` constraints in the opam solver - this allows to resolve
  more involved version constraints that were failing before (tarides/opam-monorepo#396, @samoht)

### Deprecated

### Fixed

- Error in case where multiple packages with different dev-repo fields would be
  placed in the same duniverse directory (tarides/opam-monorepo#377, @gridbugs)

- Fix a failure when using opam-monorepo with an opam 2.2 root
  (tarides/opam-monorepo#379, @kit-ty-kate)

- Fix assertion failure when prefix of "lock" subcommand is used (tarides/opam-monorepo#381,
  @gridbugs)

- Treat packages without build commands as virtual only if also lack install
  commands, as some non-virtual packages might only have install commands.
  (tarides/opam-monorepo#376 @Leonidas-from-XIV, @gridbugs)

- Improve the ordering of package candidates by putting broken packages at
  then end of the list (tarides/opam-monorepo#395, tarides/opam-monorepo#397, @samoht)

### Removed

### Security
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.

2 participants