Build system rehaul, use of Docker for CI & artifact generation on Github #299

AltGr · 2022-07-29T14:49:07Z

No description provided.

denismerigoux · 2022-07-29T16:02:08Z

Wouahou ! So how does this work ? Will the generated files be updated nightly by a bot ? Or appear in a source code release attached to the GitHub page ? I put those generated assets under Git because I wanted for somebody that wanted to just use Catala to be able to use the assets directly without having to install the compiler to generate them.

AltGr · 2022-07-29T16:07:55Z

Aah I didn't know!

But indeed, providing them as artifacts would be pretty easy: they are already generated by the CI so extracting them from the Docker image and feeding them to Github artifacts / release assets should be fairly straightforward.

Is there already a list of artifacts that we should extract in that way ?

denismerigoux · 2022-07-29T16:13:05Z

So for me, ideally:

All of french_law plus their runtime dependency as a source code zip ;
Linux/Windows/Mac x86 executables of the Catala compiler (without Z3) + Clerk ;
A Javascript binary of the Catala compiler (without JS)
You can take a look at the assets I put in the last release.

A push-button release process would be amazing, I would release more often that today with that.

AltGr · 2022-07-29T16:16:12Z

Ok I'll handle that

denismerigoux · 2022-07-29T16:16:18Z

See #219 where I talked a bit about this

The files where manually generated through Makefile rules, and versionned (with an outdated version). The issue was that we had: - `dune` building Catala - Makefiles calling `catala` to build and copy the `french_law/ocaml/law_source` files - then `dune` again to build `french_law` The result was that `dune build` (without running `make` first) would return a weird error. The proposed solution adds ad-hoc dune rules to call the catala binary, so that it can handle the whole pipeline correctly. If OCaml is purely a backend, though, a simpler solution that makes us less dependent on dune would be to handle the compilation of `french_law` manually. The dune rules are set to 'promote' the files so that it preserves the fact that they are versionned (but with no confusion of the build system about where they should come from anymore)

(plus other small Makefile improvements)

Dune has 2 default build profiles: `dev` and `release`, which change things like warnings-as-errors, and debug in jsoo build mode. We need to be in release mode when we generate the javascript artifacts (e.g. `make website-assets`), so the master `Makefile` had `--profile=release` for the `dune` commands building them. However, that rebuilds _the whole tree of dependencies_ in release mode, which means `make all` might rebuild everything several times, as the `release` flag is turned off and on. I had conflicting plugins that weren't rebuilt with the correct flag. And let's not imagine the `make -j` case… This is the best solution I can think of for the time being: - set the Makefile to use dune's `release` profile by default - this ensure consistency and avoids surprises with e.g. bad (slower) artifacts generated for the website - devs can use `dune` directly when working, or set `DUNE_PROFILE` manually if they need it I am not sure yet about CI, though, since we want warnings-as-errors but also useful artifacts. Might best to build twice…

(they're no longer reformatted, at the moment, though)

We have an obvious limitation: static builds won't support plugins. We could resort to the classic method and do normal builds but on an old distrib to dodge compatibility issues. That would still be compatible with statically linking the more specific libs, like e.g. z3.

AltGr · 2022-08-03T21:00:28Z

Should now be OK; generation of the artifacts is only activated on the master branch but I tested it here. (I removed node's node_modules & python's env since then)

Compared to the first version, I leveraged dune's "promotion" feature which means I was able to keep the french_law artifacts in-tree as before while working around the issue of them getting stale or dune getting confused.

denismerigoux

That looks awesome, a well-deserved overhaul of the build system. I raised a few nitpicks, you can merge after dealing with them.

Makefile

build_release.sh

denismerigoux · 2022-08-04T10:15:56Z

build_system/dune

@@ -16,9 +16,20 @@
  ANSITerminal)
 (modules clerk_driver))

+(rule


What's that for ?

Avoid a failure when the custom_linking.sexp file doesn't exist (we default it to a file containing just ())

(we could also statically add the file to the source tree; the upside of that would be that it is harder to get confused because you had a stale file sitting there; but it's not very pretty)

Ok and the custom_linking.sexp is produced by the build_release.sh script. Maybe that would be worth a comment in the dune file. I'm in favor of not adding custom_linking.sexp to the build tree.

examples/aides_logement/dune

denismerigoux · 2022-08-04T10:19:51Z

french_law/ocaml/law_source/.ocamlformat_ignore

@@ -0,0 +1 @@
+*


You did that to speed up the build, because we don't need to format generated files ?

Ah indeed that's something I wanted to discuss with you!
It feels a bit strange to me to reformat just-generated files; that also adds a run-time dependency to ocamlformat for running these rules. And it complicated things (we don't want ocamlformat and dune to compete on the resulting file).

No big speedup here (but in the case of the Python files, autopep8 is very slow)

But I guess there were reasons to do this in the first place so I am open to finding a workaround if needed!

The main reason for running ocamlformat is code readability.

You need to at least read the input type declarations to know what to feed your program. And since the pretty-printing in the backends is not ideal we run ocamlformat afterwards. @EmileRolley even calls directly ocamlformat in his new api_web backend plugin.

Indeed! Just discussed the matter quickly with @EmileRolley and we agreed to disable it and focus on improving our pretty-printer on cases where readability is bad (from a quick check, it seems o.k. on type definitions). Otherwise, our pretty-printer will be completely untested: if we don't want to bother, and use OCamlformat, I'd say go all the way, simplify our pretty-printer into a bare-printer and call ocamlformat as a lib.

What do you think ?

I'm okay with relying on our pretty-printer and improving it as needed, dropping ocamlformat altogether. Using ocamlformat as a lib is tempting but would introduce a lot of complexity as we use only a small portion of the OCaml AST. A textual pretty-printer would be simpler for maintainers to come and just change a tidbit while not having to understand everything. Plus that way we don't have to keep in sync with ocamlformat's AST changes, while OCaml textual syntax is somewhat backwards-compatible.

👍 that was our conclusion with @EmileRolley as well, even though printing and re-parsing is not very intellectually satisfying, generating an OCaml AST is totally impractical. My proposition on this point was meant as "use ocamlformat as a lib, but still going to/from string" — which isn't really an improvement, only a workaround for the potential dependency problem.

french_law/ocaml/law_source/aides_logement.ml

french_law/ocaml/law_source/dune

generate_website_assets.sh

…DME.md

AltGr force-pushed the french_law-build branch 4 times, most recently from f8abb1c to 2b9ea0d Compare July 29, 2022 15:36

AltGr force-pushed the french_law-build branch from 2b9ea0d to af7f00e Compare August 2, 2022 09:27

AltGr added 8 commits August 3, 2022 18:55

Include generation of json_schema artifacts

9db0388

(plus other small Makefile improvements)

Makefile: friendly message about python virtual env

21ac1af

Shell script tweaks

1dbcc31

Generate website assets through dune

c964495

Generate french law python artifacts through dune

289975e

(they're no longer reformatted, at the moment, though)

AltGr force-pushed the french_law-build branch 3 times, most recently from a852317 to e56a4b4 Compare August 3, 2022 17:26

Automate artifact generation

3ee6fdb

AltGr force-pushed the french_law-build branch from e56a4b4 to 3ee6fdb Compare August 3, 2022 20:43

AltGr changed the title ~~Generate french_law artifacts through dune & CI Docker image cleanup~~ Build system rehaul, use of Docker for CI & artifact generation on Github Aug 3, 2022

denismerigoux approved these changes Aug 4, 2022

View reviewed changes

AltGr force-pushed the french_law-build branch from 381ec1f to e3a2d82 Compare August 4, 2022 14:46

AltGr added 3 commits August 4, 2022 17:36

Fix relative dir in french_law artifacts ; add a note in examples/REA…

e50b567

…DME.md

Disable ocamlformat on api_web

e63860d

Improve api_web pretty-printer

922126b

AltGr force-pushed the french_law-build branch 2 times, most recently from 1bfbf3e to d4d7491 Compare August 4, 2022 16:08

Small improvements to the Python and OCaml pretty-printers

a569589

AltGr force-pushed the french_law-build branch from d4d7491 to a569589 Compare August 4, 2022 18:43

AltGr merged commit fb2e48d into CatalaLang:master Aug 4, 2022

denismerigoux mentioned this pull request Aug 9, 2022

Portable executables for Catala #219

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Build system rehaul, use of Docker for CI & artifact generation on Github #299

Build system rehaul, use of Docker for CI & artifact generation on Github #299

AltGr commented Jul 29, 2022

denismerigoux commented Jul 29, 2022

AltGr commented Jul 29, 2022

denismerigoux commented Jul 29, 2022

AltGr commented Jul 29, 2022

denismerigoux commented Jul 29, 2022

AltGr commented Aug 3, 2022

denismerigoux left a comment

denismerigoux Aug 4, 2022

AltGr Aug 4, 2022

AltGr Aug 4, 2022

denismerigoux Aug 4, 2022

denismerigoux Aug 4, 2022

AltGr Aug 4, 2022

denismerigoux Aug 4, 2022

denismerigoux Aug 4, 2022

AltGr Aug 4, 2022

denismerigoux Aug 4, 2022

AltGr Aug 4, 2022

Build system rehaul, use of Docker for CI & artifact generation on Github #299

Build system rehaul, use of Docker for CI & artifact generation on Github #299

Conversation

AltGr commented Jul 29, 2022

denismerigoux commented Jul 29, 2022

AltGr commented Jul 29, 2022

denismerigoux commented Jul 29, 2022

AltGr commented Jul 29, 2022

denismerigoux commented Jul 29, 2022

AltGr commented Aug 3, 2022

denismerigoux left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment