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

What's the reasoning behind filtering packages that do not have a dev-repo field? #341

Closed
kit-ty-kate opened this issue Oct 24, 2022 · 3 comments · Fixed by #362
Closed

Comments

@kit-ty-kate
Copy link
Contributor

It filters out virtual packages and packages with unknown dev-repo. *)

This took 2 hours out of my life to figure out why it wasn't adding a custom package to my mirage project.
I'm not sure I understand the reasoning behind this. This seems to have been added in #131 but I'm not sure to understand why opam-monorepo silently ignores packages that do not have a dev-repo field.

@Leonidas-from-XIV
Copy link
Member

I am not sure myself but I believe I have a decent hypothesis why this happens based on this test:

make_test ~name:"Aggregates repos"
~summaries:
[
summary_factory ~name:"y" ~version:"v" ~url_src:(Other "u") ~dev_repo:"d" ();
summary_factory ~name:"y-lwt" ~version:"v" ~url_src:(Other "u") ~dev_repo:"d" ();
]
~expected:
(Ok
[
{
dir = "d";
url = Other "u";
provided_packages =
[ { name = "y-lwt"; version = "v" }; { name = "y"; version = "v" } ];
};
])
();

opam-monorepo only supports one tarball per dev-repo, because if there are multiple tarballs, their contents might have overlapping source trees (and in fact, all opam packages from the same dev-repo released with dune-release share the same tarball which contains the entire source tree), so it needs to pick one or decide how to merge multiple source trees (which I would say is quite difficult, especially if the tarballs correspond to different versions of the dev-repo). The way it does so is by determining the dev-repo, but if they are missing the field, then it can't figure out which tarballs to aggregate.

It might be useful to display a warning if a package is missing the dev-repo, however this might be printing a lot of warnings for conf-packages or similar, so it would need to make sure to only warn on "normal" packages.

@kit-ty-kate
Copy link
Contributor Author

It might be useful to display a warning if a package is missing the dev-repo, however this might be printing a lot of warnings for conf-packages or similar, so it would need to make sure to only warn on "normal" packages.

conf packages usually have the conf flag so they could be easily filtered off this way.

@Leonidas-from-XIV
Copy link
Member

That's true; certainly worth a try implementing a warning to see whether it will output some useful messages.

Leonidas-from-XIV added a commit to Leonidas-from-XIV/opam-repository that referenced this issue 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 issue 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 issue 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 issue 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 a pull request may close this issue.

2 participants