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

Releases the dune-built piqi and piqilib #21947

Merged
merged 9 commits into from
Aug 9, 2022
Merged

Conversation

ivg
Copy link
Member

@ivg ivg commented Aug 5, 2022

On behalf of @alavrik I am releasing the piqi and piqlib packages built with dune. The changes in these versions are minimal and contain only the build system overhaul (in addition to @drvink janestreet-style compatibility changes). The main reason for switching to dune was enabling dynamic loading (before the packages didn't contain the cmxs files).

@ivg
Copy link
Member Author

ivg commented Aug 5, 2022

TL;DR; Resolved :)

Okay, this error is very weird and I can't reproduce it,

#=== ERROR while installing piqi.0.7.8 ========================================#
# context              2.2.0~alpha~dev | linux/x86_64 | ocaml-base-compiler.4.14.0 | pinned(https://github.com/alavrik/piqi-ocaml/archive/v0.7.8.tar.gz)
# path                 ~/.opam/4.14/.opam-switch/build/piqi.0.7.8
# command              ~/.opam/opam-init/hooks/sandbox.sh install dune install -p piqi,piqirun
# exit-code            1
# env-file             ~/.opam/log/piqi-7-97f291.env
# output-file          ~/.opam/log/piqi-7-97f291.out
### output ###
# Error: The following <package>.install are missing:
# - _build/default/piqi.install
# - _build/default/piqirun.install
# Hint: try running: dune build [-p <pkg>] @install

We had a similar issue in GitHub actions, which was resolved by adding the -p option to the install command as well. Now it is failing on opam-ci even with the -p options added to dune install. Note, that I can't reproduce this behavior on any machine, either installing via a pin or via a local opam-repository. Do you know what's going on here?

As a side note, piqi requires a separate and explicit install field, as it contains two dune packages, piqi and piqilib.

@ivg
Copy link
Member Author

ivg commented Aug 5, 2022

Aha, the problem was that in the opam-repository we were missing the @install target, which was the culprit. I was even able to reproduce it locally with

opam install piqi
opam reinstall --with-test piqi

Still weird why it happens only on the reinstall phase. But anyways, hope it will work this time.

In fact, deeper analysis showed that the problem is more in dune,
which incorrectly interprets the META file and doesn't see the meta
package.

This reverts commit b3ded82.
@ivg
Copy link
Member Author

ivg commented Aug 5, 2022

I reverted the bap-piqi changes, as deeper analysis showed that those packages can be built with piqi.0.7.8 and in fact we generate the same META file, modulo description. The culprit was that dune is reading the META file differently than ocamlfind and is not able to see or use the top-level package. Probably it is a bug in dune, or a feature. In any case, apparently, this release doesn't contain any breaking changes, so there is no need to add extra constraints.

]

install: [
["dune" "install" "-p" "piqi,piqirun"]
Copy link
Member

Choose a reason for hiding this comment

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

if they are both installed in the same package, why are they different dune packages? It would simplify things a lot to just have one package or to separate piqi and piqirun in two separate packages if you prefer that way

Copy link
Member Author

Choose a reason for hiding this comment

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

Because of the dune naming constraints.

  1. We need to install piqirun.pb and piqirun.ext, because these are the names that are used in downstream packages to link with the runtime. Dune requires libraries that all public libraries have a name that starts with a package name. So we had to add the piqilib package.

  2. The piqi package contains the piqi compiler itself, and we can't put into the same piqilib package, because dune or opam will complain that the released package named and the package in the project differ.

And yes, splitting the opam piqi package into piqirun and piqi should be possible, and I think that in the longterm it would be a good idea to do this. But this is @alavrik's call, I just tried to switch to dune with minimal disruption. Basically, I tried to replicate the exact behavior of the current packages.

Copy link
Member Author

@ivg ivg Aug 5, 2022

Choose a reason for hiding this comment

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

We can wait for @alavrik's opinion. If he agrees to add an extra piqirun package to the piqi universe, I will happily split it. The piqi package will depend on piqirun so it will transitively bring piqirun and we won't break anything downstream. And the opam files will be so simplified that we could in the future just do opam-release.

@alavrik
Copy link
Contributor

alavrik commented Aug 8, 2022

@ivg @kit-ty-kate thank you for your input! I am ok with any solution as long as the git repos stay as they are. The piqi and piqirun components are tightly interlocked. piqi includes the piqic-ocaml commands which is a compiler. And piqirun includes the runtime that the generated code relies on. For this reason, I believe that both pieces logically belong to a single codebase/repo.

To be completely honest, I am not well versed with the latest ocaml tooling. I would greatly appreciate your help with figuring out how to make tools to cooperate.

@ivg
Copy link
Member Author

ivg commented Aug 9, 2022

Okay, let's keep this release as it is, i.e., without introducing extra packages. The idea was to do a minimal change that switches the build system to dune and replicates the existing layout as much as possible, so let's stick to it. With that in mind, I would actually suggest in the next release to perform a restructure that is a bit more massive. Dune is very well-suited for monorepos so it would be practical to merge piqi and piqic-ocaml (which map to piqilib and piqi+piqirun) into a single git repository. And then use dune-release as the main release tool and release three opam packages with the same names. We will need to synchronize the package versions, though. Merging the repositories back together will allow us to move the bootstraping and testing to dune (as currently there's still some interproject dependencies in the build system that are not supported by dune). But we will discuss it separately with @alavrik, but right now, @kit-ty-kate would you mind merging this? I don't see any failures that are relevant to this release.

@kit-ty-kate
Copy link
Member

Thanks!

@kit-ty-kate kit-ty-kate merged commit 047bed7 into ocaml:master Aug 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants