mirage / ocaml-solo5

Freestanding OCaml runtime
Other
100 stars 30 forks source link

OCaml 4.07 support #39

Closed hannesm closed 6 years ago

hannesm commented 6 years ago

Relevant changes in 4.07

this compiles, but has not been extensively tested due to ppx issues in 4.07 (and thus unable to build the mirage tool, but working on some hacks for that)

hannesm commented 6 years ago

I force-pushed this branch - now that 4.07.0 is released (and a ocaml-src.4.07.0 is available in opam-repository). I couldn't figure out the levels of escape to inject a $ from configure.sh to ocaml-freestanding.pc (4.07.0 bundles the bigarray C stubs with stdlib, no need to build otherlibs anymore), instead I added -L${libdir} to ocaml-freestanding.pc.in and refer to nolibc, asmrun and openlibm using -l, and in case <4.07.0 is used, a -lotherlibs is added (via PKG_CONFIG_EXTRA_LIBS generated by configure.sh, stored in Makeconf, included by Makefile which does the sed in ocaml-freestanding.pc.in). I tested the changes with a 4.06.1 compiler on FreeBSD (using clang lld LLD 6.0.0 (FreeBSD 326565-1200002) (compatible with GNU linkers)).

hannesm commented 6 years ago

this links and nearly works, but linking a MirageOS unikernel requires one more symbol in 4.07.0 land (which is part of otherlibs - which we avoid to compile and use in this PR): caml_ba_map_file -- when I manually add this to nolibc/stubs.c (as STUB_ABORT(caml_ba_map_file)) I get a unikernel to link and run - but I can't find a good way to include OCaml-runtime-version specific stubs, any suggestions? -- the OCaml include files do not define an API/runtime version AFAICT, we could inject such a definition at configure.sh time, but I'm not sure about that.

NOTE: please take a look at ecd2515 for the solution using configure.sh mentioned above.

mato commented 6 years ago

@hannesm IIUC this depends on a fix for mirage/mirage#911 before I can meaningfully test it?

hannesm commented 6 years ago

yes, Drup fixed mirage on 4.07.0 with https://github.com/mirage/functoria/pull/153 and https://github.com/mirage/mirage/pull/912 - which I plan to merge soon

mato commented 6 years ago

@hannesm I've tested this (using --dev-repo pins of mirage and functoria) and it seems fine. What is the issue you were trying to fix with shell escaping?

Regarding the change in library ordering in the .pc file, this should actually be fine as long as no libraries that depend on libotherlibs come after it in the link step. As far as I can tell, that should never happen.

Modifying nolibc/stubs.c at build time is a bit hacky, but it'll do for now.

hannesm commented 6 years ago

What is the issue you were trying to fix with shell escaping?

in current master, the pc file contains ${libdir}/otherlibs.a. 4.07.0 no longer needs otherlibs.a as mentioned above. I initially wanted to add the -l${libdir}/otherlibs.a optionally (<4.07.0) via configure.sh, but I failed to get the $ sign from configure.sh via Makeconf and ocaml-freestanding.pc rule in Makefile to the generated pc. As a workaround (or fix!?), I use -L${libdir} and don't prefix the to-be-linked libraries with ${libdir} (-lotherlibs instead of ${libdir}/libotherlibs.a).

hannesm commented 6 years ago

NB functoria 2.2.1 and mirage 3.1.1 are now released to opam-repository -- which support OCaml 4.07.

mato commented 6 years ago

I had another look at this; the fact that configure.sh modifies nolibc/stubs.c is a bit annoying when working out of a Git tree, since it a) dirties the tree and b) is not idempotent. Could you move that specific action into Makefile, under the build/nolibc/Makefile target and modify the copy in build/ instead? You should be able to use ifeq to make it conditional on 4.07.0, and make clean will clean up the change.

hannesm commented 6 years ago

@mato done in 0d41cd9 (and tested locally with both 4.06.1 and 4.07.0)

hannesm commented 6 years ago

CI is broken due to https://github.com/ocaml/ocaml-ci-scripts/pull/232 which now uses opam2 (thx to @samoht - see https://discuss.ocaml.org/t/ocaml-ci-scripts-are-now-using-opam2-by-default/2379 as well), not sure what to do here - we can tell travis to use opam1, or update the opam file to opam2. what do you think?

hannesm commented 6 years ago

given that the main opam-repository will only switch to 2.0.0 on september 17th (according to https://opam.ocaml.org/blog/opam-2-0-0-repo-upgrade-roadmap/), I guess it is wise to use opam1 until then here as well (to avoid having to downgrade opam files for potential new releases) -- I'll shortly add a commit here to stick travis to opam1

samoht commented 6 years ago

@hannesm what was the error with the CI script and opam2? I can help fixing those :-)

hannesm commented 6 years ago

@samoht from https://travis-ci.org/mirage/ocaml-freestanding/jobs/413023314

opam lint opam --warn=-21-32-48
/home/travis/build/mirage/ocaml-freestanding/opam: Errors.
    error 55: Non-normalised OS or arch string being tested: "amd64 (use x86_64
              instead)", "amd64 (use x86_64 instead)", "aarch64 (use arm64
              instead)"
'unset TESTS; OPAMYES=1 export OPAMYES; set -ue; opam lint opam --warn=-21-32-48' exited 1. Terminating with 1

it's easy enough to fix, but will break opam1..

samoht commented 6 years ago

Or we can also add -55 to the list of ignored warnings.

hannesm commented 6 years ago

I'm not a big fan about ignoring warnings in the CI scripts -- it is usually hard to get rid of once ignored warnings (similar to removing LATEST https://github.com/ocaml/ocaml-ci-scripts/issues/110), since it leads to spurious issues at random PRs..

mato commented 6 years ago

@hannesm Thanks, this all looks good to me, including sticking with OPAM 1.x for now.