search

mirage / ocaml-solo5

Freestanding OCaml runtime
Other
100 stars 30 forks source link

Build system cleanups #65

Closed hannesm closed 4 years ago

hannesm commented 5 years ago

see individual commits for details if interested (but first take a look whether CI is happy). comments welcome if there are controversial changes (such as calling OCaml's configure). Tested with 4.08.1 and 4.09.0

samoht commented 4 years ago

I haven't spotted any obvious issues, but it's hard to say with these scripts :-) So I'm good, as long as the CI is happy...

mato commented 4 years ago

I'll take a detailed look next week.

We are missing CI for non-Linux platforms, so this is somewhat fragile. FWIW, Cirrus CI supports at least FreeBSD, worth integrating here and possibly elsewhere? Patches welcome :-)

https://cirrus-ci.org/guide/FreeBSD/

hannesm commented 4 years ago

what I learned from doing these "cleanups" (motivated by e.g. #63) is that once we decide to drop < 4.08.0 we can simplify ocaml-freestanding quite drastically (removing all the generated and checked in makefiles and m.YY.h -- the s.h still being provided by ocaml-freestanding and copied over to the OCaml directory). And that I'm not sure about the current 4.08.0+ support in this repository, as the OCaml build system changed quite radically (moved to autotools which implied renamings, it seems we're lucky and provide the CFLAGS via OC_CFLAGS) -- I'd be interested if there's any argument against using OCaml's ./configure while building ocaml-freestanding.

with OCaml 4.10 being frozen, and MirageOS subscribed to "the last two major releases", I personally don't have any issue dropping all < 4.08.0 pretty soon (although we should "stay in sync" with xen for now to have some common supported runtimes).

mato commented 4 years ago

@hannesm Mind re-basing against latest master to get your Cirrus CI for FreeBSD?

hannesm commented 4 years ago

@mato thanks for your review. I can remove the calling configure commit from this PR, and have that as a separate PR?

I agree it is wrong to call configure this way (but also argue that the current checked-in configure output is brittle as seen in #63, and only works by accident).

mato commented 4 years ago

I can remove the calling configure commit from this PR, and have that as a separate PR?

Let's just do it here, I have something that looks like it may work.

mato commented 4 years ago

@hannesm Take a look at cca57cb. Assuming CI passes, I think this is a workable solution for fully using autotools when on OCaml >= 4.08.0.

hannesm commented 4 years ago

I tested locally (thanks @mato) and have some observations and questions: 1) the resulting libasmrun.a is much bigger (1.5 MB instead of 470KB) -- since -g is now passed -- a strip on libasmrun.a pushes it back to ~200KB (this is only an observation, it is fine) 2) the arguments passed to configure -- I'm not entirely sure which ones are needed and which ones are not (we're only using the libasmrun.a target) -- for me ./configure --host=$(BUILD_ARCH)-unknown-none (i.e. no further arguments) works as well and produces the same libasmrun.a 3) the amount of conditionals in Makefiles and shell scripts for 4.06 and 4.07 support let's me wonder whether we should just drop that -- users who require these compilers can use older ocaml-freestanding releases (e.g. 0.4.7).

mato commented 4 years ago 
1. the resulting `libasmrun.a` is much bigger (1.5 MB instead of 470KB) -- since `-g` is now passed -- a `strip` on `libasmrun.a` pushes it back to ~200KB (this is only an observation, it is fine)

This is fine and expected (configure defaults to -g).

2. the arguments passed to configure -- I'm not entirely sure which ones are needed and which ones are not (we're only using the `libasmrun.a` target) -- for me `./configure --host=$(BUILD_ARCH)-unknown-none` (i.e. no further arguments) works as well and produces the same `libasmrun.a`

That was me experimenting with configure invocations. Removed.

3. the amount of conditionals in Makefiles and shell scripts for 4.06 and 4.07 support let's me wonder whether we should just drop that -- users who require these compilers can use older ocaml-freestanding releases (e.g. 0.4.7).

We can surely drop this, perhaps one release after the one with this change?

hannesm commented 4 years ago

We can surely drop this, perhaps one release after the one with this change?

fine with me. just keeping it in my head, before investing more time into the 4.06/4.07 compatibility, just remove it. since this PR works on 4.06/4.07, I see no reason to drop it immediately.

From my point of view & review, this PR is ready to be merged (if CI passes). if you like to squash all commits / reorder / squash some commits, please go ahead. Also feel free to merge at your convenience.