tarides / dune-release

Streamlining the release of dune packages to opam
ISC License
115 stars 37 forks source link

dune-release fails with multi-opam repository #420

Open hannesm opened 2 years ago

hannesm commented 2 years ago

When attempting to use dune-release on happy-eyeballs (https://giithub.com/roburio/happy-eyeballs), I get errors. A workaround is to specify -p happy-eyeballs (release a single package only). The underlying reason seems to be that dns-client depends on happy-eyeballs, and happy-eyeballs-lwt/happy-eyeballs-mirage depend on dns-client.lwt/.mirage (see https://github.com/ocurrent/ocaml-ci/issues/381 for a similar tooling issue).

$ dune-relesae check
[-] Checking dune-release compatibility.
[ OK ] The dev-repo field of happy-eyeballs.opam contains a github uri.
[ OK ] The dune project contains a name stanza.

[-] Building package in _build/.dune-release-check
Error: Conflict between the following libraries:
- "happy-eyeballs" in _build/default/src
- "happy-eyeballs" in /usr/home/hannes/.opam/4.13.0/lib/happy-eyeballs
  -> required by library "dns-client.lwt" in
     /usr/home/hannes/.opam/4.13.0/lib/dns-client/lwt
  -> required by library "happy-eyeballs-lwt" in _build/default/lwt
-> required by executable test in app/dune:2
Error: Conflict between the following libraries:
- "happy-eyeballs" in _build/default/src
- "happy-eyeballs" in /usr/home/hannes/.opam/4.13.0/lib/happy-eyeballs
  -> required by library "dns-client.lwt" in
     /usr/home/hannes/.opam/4.13.0/lib/dns-client/lwt
Error: Conflict between the following libraries:
- "happy-eyeballs" in _build/default/src
- "happy-eyeballs" in /usr/home/hannes/.opam/4.13.0/lib/happy-eyeballs
  -> required by library "dns-client.mirage" in
     /usr/home/hannes/.opam/4.13.0/lib/dns-client/mirage

[FAIL] package(s) build
...
pitag-ha commented 2 years ago

Hey @hannesm, thanks for reporting this! Do I understand correctly that

dune build -p happy-eyeballs
dune build -p happy-eyeballs-lwt
dune build -p happy-eyeballs-mirage

separately all work well, while

dune build -p happy-eyeballs,happy-eyeballs-lwt,happy-eyeballs-mirage

returns the error you've copied since it will first build the happy-eyeballs library into the dune directory and then look for a unique happy-eyeballs library when building the other two packages (and also find one in the switch)?

If I understand the situation correctly, I think that for the tarball check, dune-release should iterate over separate package builds (dune build -p pkg_i for all packages pkg_i) and tests instead of joining them (dune build -p pkg_1,...,pkg_n), which should be a simple fix.

hannesm commented 2 years ago

Hi @pitag-ha,

dune build -p happy-eyeballs dune build -p happy-eyeballs-lwt dune build -p happy-eyeballs-mirage

separately all work well

Yes, indeed.

while

dune build -p happy-eyeballs,happy-eyeballs-lwt,happy-eyeballs-mirage

returns the error you've copied

yes.

which should be a simple fix.

That would be great. I guess for building documentation each package should be built separately as well. Thanks!

pitag-ha commented 2 years ago

Cool.

I guess for building documentation each package should be built separately as well.

Oh, that's a very good point! So in the end it seems like that fixing this will require more work and even a change in dune-release's behavior. Currently, it builds all the documentation at once and pushes that to one github page. Whereas what you're saying is to have one github page per package, right?

hannesm commented 2 years ago

Not sure what should be done in respect to the documentation -- certainly a dune build @doc -p happy-eyeballs ; dune build @doc -p happy-eyeballs-lwt ; dune build @doc -p happy-eyeballs-mirage followed by a single push to gh-pages should be fine :)

pitag-ha commented 2 years ago

Aah, I see. Thanks! (I'll be on new year's vacation from tomorrow on, but I can see if it's as simple as I think when I'm back)

hannesm commented 2 years ago

Hi there, I ended up here again when attempting to cut a release of happy-eyeballs. Is there any simple fix? Thanks a lot.

NathanReb commented 2 years ago

Have you tried removing the packages from your opam switch before running this? I.e. run:

opam remove happy-eyeballs happy-eyeballs-lwt happy-eyeballs-mirage

Since the packages are interdependent I think it make sense that the build checks that they can be built together rather than build them using the opam installed versions. This is especially true if you release new features in one of the packages mentioned, the opam installed version clearly won't have it. I think the problem here is that the -p option in dune should clarify that the local packages should be used instead of opam installed ones, clearing out the conflict.

To be very clear, running three separate -p commands with individual packages won't work unless you pin the other two to the development version you're trying to release, which is just another way of running -p pk1,pk2,pk3.

I'll report this on dune.

Another temporary workaround you can use here, if you're confident in your release, is to pass --skip-build --skip-test. I understand this is not the most satisfying solution but it can help get through with the release if the above does not work for some reason.

NathanReb commented 2 years ago

Ah my bad I missed the intermediate dns-client package here. Do you still get the error if you pin happy-eyeballs' repo before running the release?

hannesm commented 2 years ago

Do you still get the error if you pin happy-eyeballs' repo before running the release?

Yes.

NathanReb commented 2 years ago

I assume the pinning won't solve your problem here unfortunately. The "interleaved" structure of the happy-eyeball and ocaml-dns multi-opam repos is a bit of a problem here.

The only viable solution I could see would be to indeed run the build for each package separately but this can only reliably work if you pinned them to the exact version you're releasing. dune-release currently does not handle the setup for the tests and I'm not sure it should change. Switching the command used to run the builds and test on the release tarball could be considered a breaking change as users that have simpler multi-opam repos would now have to pin the repo before releases. I'm a bit torn but I guess this approach is more robust but we'll need to properly document this and make it part of the 2.0.0 release.

NathanReb commented 2 years ago

To be entirely fair I think this kind of testing would also be better handled in a CI than locally but I guess we'll have to settle for baby steps at first as this needs fixing!

hannesm commented 2 years ago

Thanks for your investigations. What I do for development is to symlink my local ocaml-dns directory as part of the happy-eyeballs directory. This allows dune build to succeed. But dune-release seems to do something different, and complains with the above error.

NathanReb commented 2 years ago

That's because dune-release uses the content of the tarball to run the builds, which likely doesn't include your local copy of ocaml-dns since it is built using the git revision for the release tag.

The more I think about this, the more I think it in fact makes sense to run the build and test commands separately and rely on users pinning the repo correctly. This is what our old CI scripts used to do and what is closest to what opam install will do. Since users already have to provide a valid switch for this to work I think it's okay to let them handle the pinning, as long as we properly document it. Using the opam libraries we could also warn users when their switch is not properly configured.