Fix: merge multiple identical opam pins on a local package #13232
Conversation
ElectreAAS
force-pushed
the
push-rqpyxukqkprq
branch
from
e7c5163 to
1d609d2
Compare
| We pick the opam version here. | ||
| $ dune_pkg_lock_normalized | ||
| Solution for dune.lock: | ||
| - trouble.0.0.1 |
There was a problem hiding this comment.
This looks very dubious to me, especially since there is this comment in a related test:
There was a problem hiding this comment.
Yes, this is quite dubious. I would say if the versions disagree we should error and not pick sides, to be consistent with how it works when it is pinned twice via dune.
But this isn't introduced by this code, right? In such case it could be fixed in a separate PR.
There was a problem hiding this comment.
Indeed this wasn't introduced by this PR, it was already present behaviour
There was a problem hiding this comment.
Why is it dubious? This is exactly what we do everywhere else. The opam pins are there for opam users presumably.
There was a problem hiding this comment.
The comment in mixed-opam-dune says we should favor the dune metadata when there is both opam and dune info
Here that is not what is happening, we favor the opam info
If you think that's fine, I'll leave it as is
|
Usability note: the bug was discovered while trying to build irmin, and I can confirm that with this patch irmin builds correctly! |
|
The errors seem to be non deterministic? Perhaps you need to sort them? |
Leonidas-from-XIV
left a comment
Leonidas-from-XIV
left a comment
There was a problem hiding this comment.
I think the code is fine, the main question is whether that dubious behavior comes from this PR or not.
| We pick the opam version here. | ||
| $ dune_pkg_lock_normalized | ||
| Solution for dune.lock: | ||
| - trouble.0.0.1 |
There was a problem hiding this comment.
Yes, this is quite dubious. I would say if the versions disagree we should error and not pick sides, to be consistent with how it works when it is pinned twice via dune.
But this isn't introduced by this code, right? In such case it could be fixed in a separate PR.
test/blackbox-tests/test-cases/pkg/pin-stanza/pin-depends-multiple.t
Outdated
Show resolved
Hide resolved
ElectreAAS
force-pushed
the
push-rqpyxukqkprq
branch
from
1d609d2 to
8dea19a
Compare
Signed-off-by: Ambre Austen Suhamy <ambre@tarides.com>
ElectreAAS
force-pushed
the
push-rqpyxukqkprq
branch
from
8dea19a to
1a1d444
Compare
src/dune_pkg/pinned_package.ml
Outdated
| [ Pp.textf "local package %s cannot be pinned" (Package_name.to_string pin.name) ] | ||
| ~loc:loc1 | ||
| [ Pp.textf | ||
| "local package %s is pinned twice with different versions" |
There was a problem hiding this comment.
One improvement that could be made here in a follow up would be to mention the versions. Something like Error: local package trouble is pinned with version 9.9.9 but also version 1.0.0 in main_right.opam:1
|
When you merge, could you update the commit description (GitHub should prompt you) with some more detailed information. Reading it now it is not so clear. |
Signed-off-by: Ambre Austen Suhamy <ambre@tarides.com>
ElectreAAS
force-pushed
the
push-rqpyxukqkprq
branch
from
1a1d444 to
5884666
Compare
4 participants
Fixes #12990
Commit 1 expands the test
Commit 2 fixes one bug, but there is remaining weirdness going on
Commit 3 is just what felt to me like a readibility improvement, completely optionaldeleted as not wanted