Skip to content

OCaml 4.07 support#39

Merged
mato merged 4 commits into
mirage:masterfrom
hannesm:4.07
Aug 9, 2018
Merged

OCaml 4.07 support#39
mato merged 4 commits into
mirage:masterfrom
hannesm:4.07

Conversation

@hannesm
Copy link
Copy Markdown
Member

@hannesm hannesm commented Jun 20, 2018

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)

Comment thread ocaml-freestanding.pc.in Outdated
Description: Freestanding OCaml runtime
Cflags: -I${includedir}
Libs: ${libdir}/libasmrun.a ${libdir}/libotherlibs.a ${libdir}/libnolibc.a ${libdir}/libopenlibm.a @@PKG_CONFIG_EXTRA_LIBS@@
Libs: ${libdir}/libasmrun.a ${libdir}/libnolibc.a ${libdir}/libopenlibm.a @@PKG_CONFIG_EXTRA_LIBS@@
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

i'm not entirely sure about this -- is the order of the .a archives relevant?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

it is not, but the escaping in configure.sh needs some additional \

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm not sure what you mean. The archives should be listed in dependency order, but your change doesn't change that.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ah, I see now, you move libotherlibs.a into PKG_CONFIG_EXTRA_LIBS for OCaml < 4.07.0. That might break some linkers.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

yes, and escaping the $ from configure.sh via Makeconf and Makefile doesn't seem to be possible for me.... we may need to introduce some other mechanism

Relevant changes in 4.07
- unix.c now #include <sys/ioctl.h> (since 852b595ff3), but an empty one is fine
  (only use is in caml_nun_rows_fd in an #ifdef TIOCGWINSZ), see
  ocaml/ocaml#1431
  -> provide an empty ioctl via nolibc
- bigarray is now part of the stdlib
  ocaml/ocaml#1685
  -> libotherlibs.a is no longer needed (neither built nor linked)

To support these changes, an empty sys/ioctl.h is added to nolibc.  Also,
`ocaml-freestanding.pc.in` now contains in `Libs`: -L${libdir} -lasmrum -lnolibc
-lopenlibm (instead of ${libdir}/lib for each library).  `configure.sh` adds
-lotherlibs to these flags in the case that OCAML_GTE_4_07_0 is false.  The
`Makefile` treats libotherlibs.a as dependency only if OCAML_GTE_4_07_0 is false.
@hannesm hannesm changed the title WIP: OCaml 4.07 support READY: OCaml 4.07 support Jul 14, 2018
@hannesm
Copy link
Copy Markdown
Member Author

hannesm commented Jul 14, 2018

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 hannesm changed the title READY: OCaml 4.07 support READY for review: OCaml 4.07 support Jul 14, 2018
@hannesm
Copy link
Copy Markdown
Member Author

hannesm commented Jul 18, 2018

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
Copy link
Copy Markdown
Contributor

mato commented Jul 19, 2018

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

@hannesm
Copy link
Copy Markdown
Member Author

hannesm commented Jul 19, 2018

yes, Drup fixed mirage on 4.07.0 with mirage/functoria#153 and mirage/mirage#912 - which I plan to merge soon

@mato
Copy link
Copy Markdown
Contributor

mato commented Jul 29, 2018

@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
Copy link
Copy Markdown
Member Author

hannesm commented Jul 29, 2018

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
Copy link
Copy Markdown
Member Author

hannesm commented Aug 1, 2018

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

@mato
Copy link
Copy Markdown
Contributor

mato commented Aug 6, 2018

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
Copy link
Copy Markdown
Member Author

hannesm commented Aug 7, 2018

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

@hannesm
Copy link
Copy Markdown
Member Author

hannesm commented Aug 7, 2018

CI is broken due to ocaml/ocaml-ci-scripts#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
Copy link
Copy Markdown
Member Author

hannesm commented Aug 7, 2018

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
Copy link
Copy Markdown
Member

samoht commented Aug 7, 2018

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

@hannesm
Copy link
Copy Markdown
Member Author

hannesm commented Aug 7, 2018

@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
Copy link
Copy Markdown
Member

samoht commented Aug 7, 2018

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

@hannesm
Copy link
Copy Markdown
Member Author

hannesm commented Aug 7, 2018

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 ocaml/ocaml-ci-scripts#110), since it leads to spurious issues at random PRs..

@mato mato changed the title READY for review: OCaml 4.07 support OCaml 4.07 support Aug 9, 2018
@mato
Copy link
Copy Markdown
Contributor

mato commented Aug 9, 2018

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

@mato mato merged commit 7233274 into mirage:master Aug 9, 2018
@hannesm hannesm deleted the 4.07 branch August 13, 2018 17:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants