Closed kit-ty-kate closed 3 years ago
thanks for your patch, I approve the first commit (add mkdir & rmdir), but would for now delay the second commit until opam 2.1 semantics is settled (in respect to opam var
).
My reasoning behind adding --readonly
is that it is always correct to have it anyway so whether or not opam 2.1 is broken (which I would argue it is, I agree). The option itself was added in opam 2.0.0.
However if you really don't want it and want to make a new release quickly I'll remove it no problem. I'll keep it in opam-alpha-repository though as I need it for testing.
@kit-ty-kate my reasoning is "we use at 10 other places opam config var prefix
, so if we add --readonly
here, let's go the full path down and use it everywhere". it does not hurt to add it here, since opam 2.0 supports it already, as you mentioned. but it hurts a bit to have it not consistent across the packages I care about.
Fair enough, removed.
I rebased and force-pushed. The issue with a CI for OCaml 4.12 is that we need the ocaml-src
package in opam-repository first (which is debatable whether we want to have this for pre-releases).
This PR is good to go
The CI issue is (same as observed in https://github.com/mirage/mirage-tcpip/pull/439#issuecomment-771575300):
ld: /home/opam/.opam/4.08/lib/pkgconfig/../../lib/ocaml-freestanding/libasmrun.a(startup_aux_n.o):/home/opam/.opam/4.08/.opam-switch/build/ocaml-freestanding.0.6.3/ocaml/runtime/startup_aux.c:35: multiple definition of `caml_atom_table'; /home/opam/.opam/4.08/lib/pkgconfig/../../lib/ocaml-freestanding/libasmrun.a(startup_nat_n.o):/home/opam/.opam/4.08/.opam-switch/build/ocaml-freestanding.0.6.3/ocaml/runtime/startup_nat.c:47: first defined here
Interestingly, this only happens on GNU/Linux (with 4.08.1), but not on FreeBSD (see the cirrus CI run). To move forward (and not waste several days investigating build systems), I suggest to drop 4.08 support from freestanding (and MirageOS). Any objections? /cc @mato
mkdir
andrmdir
were added in https://github.com/ocaml/ocaml/pull/9797