search

mirage / mirage-solo5

Solo5 core platform libraries for MirageOS
ISC License
20 stars 21 forks source link

dune: use -Werror only in development mode, add :standard #71

Closed hannesm closed 4 years ago

hannesm commented 4 years ago

Using -Werror in released packages is brittle: C compilers tend to add new warnings with new releases (which we can't predict that we don't violate).

In development mode (this means: a local run of dune build), we keep the -Werror, but in production (i.e. opam-installed) we avoid adding -Werror. This makes released mirage-solo5 packages more robust.

In addition, we add :standard to the CFLAGS (which is -O2 -fno-strict-aliasing -fwrapv -fPIC on my platform & dune -- run dune printenv to see how it is on your platform). This is in general a good idea AFAICT (see discussion in https://github.com/mirage/mirage-crypto/pull/47).

mato commented 4 years ago

Using -Werror in released packages is brittle: C compilers tend to add new warnings with new releases (which we can't predict that we don't violate).

I get the theoretical problem (newer, stricter compiler is released), but, for the sake of argument, do we actually have a known case of this happening?

In development mode (this means: a local run of dune build), we keep the -Werror, but in production (i.e. opam-installed) we avoid adding -Werror. This makes released mirage-solo5 packages more robust.

How does this affect builds done by CI? How does this affect local development in the case of:

  1. opam pin add mirage-solo5 .
  2. Make changes.
  3. opam reinstall mirage-solo5
  4. (build something depending on the newly reinstalled mirage-solo5)

I tend to use the above workflow a lot when developing, at least as often as a local dune build.

In addition, we add :standard to the CFLAGS (which is -O2 -fno-strict-aliasing -fwrapv -fPIC on my platform & dune -- run dune printenv to see how it is on your platform). This is in general a good idea AFAICT (see discussion in mirage/mirage-crypto#47).

Some of those flags are controversial:

  1. -f-no-strict-aliasing: I'd prefer to code against the strict aliasing semantics (enabled by default at -O2 and above according to my GCC manual).
  2. -fPIC: Mirage does not use shared libraries, so producing position-independent code is pointless. Also, it will conflict if we ever want to do ASLR at the low levels, for which we would need -fpie / -fPIE.

Who made the choices of what goes into :standard, and why?

/cc @samoht

hannesm commented 4 years ago

have a known case of this happening?

I ran into such an issue with mirage-crypto (but not with mirage-solo5 yet). Feel free to close this issue, we can take action once we encounter such an issue.

opam pin

I've not checked what happens in an opam pin built, I suspect it won't be in dev mode (profile).

:standard

As you like, I'm fine without the standard flags as well. I suspect the dune developers decide on what's in there and what not. Thanks for raising the issues about incompatibilities with PIE etc. - I'll have to rethink the mirage-crypto repo in the light of this.

dinosaure commented 4 years ago

As you like, I'm fine without the standard flags as well. I suspect the dune developers decide on what's in there and what not. Thanks for raising the issues about incompatibilities with PIE etc. - I'll have to rethink the mirage-crypto repo in the light of this.

For the MirageOS dunification, this dune's variable is mandatory. From what I know, the mirage tool will set a dune workspace with right C options according the target chosen by the user and these flags will be expanded with :standard - so, at least, the strong requirement (and the only for MirageOS 4) is to use :standard for any C stubs.

hannesm commented 4 years ago

@mato

How does this affect builds done by CI? How does this affect local development in the case of:

  1. opam pin add mirage-solo5 .
  2. Make changes.
  3. opam reinstall mirage-solo5
  4. (build something depending on the newly reinstalled mirage-solo5)

for completeness, the (env (dev (c_flags (-Werror)))) would remove the -Werror from such a workflow (similarly to the CI workflow in this repository), and thus is not a good idea. For other CI systems (that do a dune build manually, instead of relying on opam), the -Werror would be active.

Thanks for the discussion and clarifications, I'm sure this PR is not the right way forward. Closing.