Better missing version error #248
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Leonidas-from-XIV
force-pushed
the
better-missing-version-error
branch
from
80311ff
to
ec87109
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
With @NathanReb's very valuable hint that I can get the rejection condition from |
Leonidas-from-XIV
force-pushed
the
better-missing-version-error
branch
from
0dbffdd
to
e93736e
Compare
|
Do you think we could extract a test for this? That would also help seeing what the output looks like! |
|
Yes, that's definitely the idea! Which is why I rebased on |
Leonidas-from-XIV
force-pushed
the
better-missing-version-error
branch
from
e93736e
to
61c89d0
Compare
This has the advantage that it will not fail when there aren't any FailsRestriction rejections.
Leonidas-from-XIV
force-pushed
the
better-missing-version-error
branch
from
61c89d0
to
a0cf396
Compare
Leonidas-from-XIV
force-pushed
the
better-missing-version-error
branch
from
9c8546f
to
09a0c06
Compare
Thanks to @emillon for helping me debug this behavior of grep. Which is somewhat understandable (given grep adds a \n and with -z it is a \0) but extremely unhelpful.
|
So I think this is ready for review. Adding these cram tests is quite rewarding and reasonably easy so I think overall they're a big win in documenting the behavior and making sure it doesn't get broken (and if it does, we will know about it). |
NathanReb
reviewed
Feb 8, 2022
There was a problem hiding this comment.
This looks good, thanks!
I have a few comments code-wise.
I also think we should add a regression test for the original issue, the one with no dune compatible versions as well. That is, if for a package, no version build with dune and no version match the constraint, only the latter is reported.
|
Also looking at the output, I think we should consider dropping the base diagnosis if we find such packages or packages that don't build with dune. We can do this in a separate PR but I'm curious to know what you think about such an approach! |
NathanReb
reviewed
Feb 9, 2022
NathanReb
reviewed
Feb 14, 2022
NathanReb
reviewed
Feb 14, 2022
Co-authored-by: Nathan Rebours <[email protected]>
Co-authored-by: Nathan Rebours <[email protected]>
Leonidas-from-XIV
force-pushed
the
better-missing-version-error
branch
from
127a98b
to
4a9621c
Compare
NathanReb
approved these changes
Feb 15, 2022
NathanReb
added a commit
to NathanReb/opam-repository
that referenced
this pull request
Apr 19, 2022
CHANGES: ### Added - Add opam extensions `x-opam-monorepo-opam-repositories` and `x-opam-monorepo-global-opam-vars` to make `lock` fully reproducible. (tarides/opam-monorepo#250, tarides/opam-monorepo#253, @NathanReb) - Show an error message when the solver can't find any version that satisfies the requested version constraint in the user's OPAM file (tarides/opam-monorepo#215, tarides/opam-monorepo#248, @Leonidas-from-XIV) - Allow packages to be marked as being provided by Opam and not to be pulled by `opam-monorepo`. To control this a new optional Opam file field, `x-opam-monorepo-opam-provided` is introduced. Its value is a list of package names that are to be excluded from being pulled (tarides/opam-monorepo#234, @Leonidas-from-XIV) - Show an error message when the OCaml version of the lock file does not match the OCaml version of the switch (tarides/opam-monorepo#267, tarides/opam-monorepo#268, @Leonidas-from-XIV) - Generate a `duniverse/README.md` file to explain the basics of `opam-monorepo` in the vendored directory (tarides/opam-monorepo#272, tarides/opam-monorepo#274, @Leonidas-from-XIV) - Add a `--prefer-cross-compile` flag for the solver to select cross-compiling versions of packages when available. This is determined through the presence of the `"cross-compile"` tag in the opam metadata. ### Changed - Bump lockfile version to 0.3 (tarides/opam-monorepo#285, @NathanReb) - Mark packages to be pulled by opam-monorepo with the `vendor` variable so using OPAM with `opam install --deps-only --locked .` will not install packages that will be installed with `opam-monorepo pull` (tarides/opam-monorepo#237, @Leonidas-from-XIV) ### Deprecated ### Fixed - Fix a bug where a package which had a single version that built with dune and got selected by the solver would be reported has having no version building with dune. (tarides/opam-monorepo#245, @Leonidas-from-XIV) - Fix the solver so it does not select beta versions of the compiler unless forced to by version constraints or `--ocaml-version`. (tarides/opam-monorepo#269, @NathanReb) ### Removed - Drop support for lockfile versions 0.2 and lower (tarides/opam-monorepo#285, @NathanReb) ### Security
NathanReb
added a commit
to NathanReb/opam-repository
that referenced
this pull request
Apr 20, 2022
CHANGES: ### Added - Add opam extensions `x-opam-monorepo-opam-repositories` and `x-opam-monorepo-global-opam-vars` to make `lock` fully reproducible. (tarides/opam-monorepo#250, tarides/opam-monorepo#253, @NathanReb) - Show an error message when the solver can't find any version that satisfies the requested version constraint in the user's OPAM file (tarides/opam-monorepo#215, tarides/opam-monorepo#248, tarides/opam-monorepo#290 @Leonidas-from-XIV) - Allow packages to be marked as being provided by Opam and not to be pulled by `opam-monorepo`. To control this a new optional Opam file field, `x-opam-monorepo-opam-provided` is introduced. Its value is a list of package names that are to be excluded from being pulled (tarides/opam-monorepo#234, @Leonidas-from-XIV) - Show an error message when the OCaml version of the lock file does not match the OCaml version of the switch (tarides/opam-monorepo#267, tarides/opam-monorepo#268, @Leonidas-from-XIV) - Generate a `duniverse/README.md` file to explain the basics of `opam-monorepo` in the vendored directory (tarides/opam-monorepo#272, tarides/opam-monorepo#274, @Leonidas-from-XIV) - Add a `--prefer-cross-compile` flag for the solver to select cross-compiling versions of packages when available. This is determined through the presence of the `"cross-compile"` tag in the opam metadata. ### Changed - Bump lockfile version to 0.3 (tarides/opam-monorepo#285, @NathanReb) - Mark packages to be pulled by opam-monorepo with the `vendor` variable so using OPAM with `opam install --deps-only --locked .` will not install packages that will be installed with `opam-monorepo pull` (tarides/opam-monorepo#237, @Leonidas-from-XIV) ### Fixed - Fix a bug where a package which had a single version that built with dune and got selected by the solver would be reported has having no version building with dune. (tarides/opam-monorepo#245, @Leonidas-from-XIV) - Fix the solver so it does not select beta versions of the compiler unless forced to by version constraints or `--ocaml-version`. (tarides/opam-monorepo#269, @NathanReb) ### Removed - Drop support for lockfile versions 0.2 and lower (tarides/opam-monorepo#285, @NathanReb)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I've continued to play around with what info can be retrieved from the map and with a bit of squinting (extracting the version restriction from another rejection case) I was able to test the version check on the packages that we exclude as not building with dune.
If I specify
fmt {>= "1.0.0"}in myopamfile I get this error message now:It is somewhat involved and hairy so I am not entirely sure whether this is worth it so I'm opening this PR as a draft, more of an RFC to see if we even want to do stuff this way or whether we should patch the 0install solver to expose better information instead.
Closes #215