Add option learn-ocaml build serve --serve-during-build and fix related minor issues

[erikmd]

• Kind: feature

• Close Feature: new option à la --replace to leverage this existing feature in a docker restart context #594

Description

Good news, @AltGr @yurug !

I was able (with Louis' advice from last week) to implement #594.

Regarding the naming of the CLI command, I now propose --serve-during-build (better than my initial try, --fork-replace)

Each commit comes with a detailed description (or motivation), in particular note that:

• I fixed a minor issue (that originated in the --replace PR) due to the fact alpine's default lsof supports no option; unlike the lsof package;
• I fixed another minor issue regarding a missing flush in Learnocaml_server.kill_running;
• and I changed the formatting of the displayed version (→ if you don't object, it will print Learnocaml v1.1.0 running instead of Learnocaml v.1.1.0 running, which was making less sense given our current git tag convention).

I thoroughly tested the new option as summarized in the message of the main commit f0108a3
but as suggested by @AltGr, I didn't turn the option on by default in the Dockerfile.

Regarding performance, I ran an experiment on learn-ocaml-corpus endowed with the following docker-compose.yml:

docker-compose.yml

services:
  lo-test:
    image: learn-ocaml  # build using ( sudo make docker-images )
    volumes:
      - ../learn-ocaml-corpus:/repository
      - lo-test-serve-during-build-sync:/sync
    ports:
      - '8080:8080'
    restart: unless-stopped
    environment:
      - 'LEARNOCAML_SERVE_DURING_BUILD=true'

volumes:
  lo-test-serve-during-build-sync:
    driver: local

along with the following script:

shell session

$ sudo make docker-images
$ sudo docker compose up -d
$ sudo docker compose logs -t lo-test
$ sudo docker compose stop
$ touch ../learn-ocaml-corpus/exercises/fpottier/*/test.ml  # touch 26 exercises
$ sudo docker compose restart
$ sudo docker compose logs -t lo-test

And I got:

• (around 10' to build the docker image locally)
• 54s to build learn-ocaml-corpus (offline)
• (<1s to start the server)
• (immediate stop)
• (<1s to restart the child server from the previous www)
• 19s to rebuild learn-ocaml-corpus/exercises/fpottier (online)
• (<1s to kill the child server and start the main one)

So I believe the effort was worth it for this quite standard use case :)

Now, let's wait for the CI ! and let me know if you have review comments.

And last hint: we shouldn't squash-merge this PR but rather do a regular merge (to preserve the atomic commits).

Checklist

• Read the CONTRIBUTING.md guide and:
  • Use Atomic Commits so each commit gathers a single logical change
  • Use Conventional Commits regarding commit messages (needed by our release toolchain)
• Add/update tests
• Add/update documentation

Note to maintainers

• Read this wiki page.
• Make sure the PR has a milestone.
• Assign yourself before merging.
• Either do a regular merge:
  • for PRs containing several commits following conventional-commits,
  • or for PRs containing 1 commit shared with a later PR (to preserve the SHA1)
• Or do a squash-merge:
  • for PRs containing only 1 commit (not shared with a later PR),
  • or for PRs containing several commits that need not be kept in the history;
  • → Update the commit message header with a conventional-commit type,
  • → Add a footer Close #… if a related issue exists.

[AltGr]

Wow, this is more complex than I anticipated.

It's fine overall but I think some points should be clarified, in particular I have trouble following the meaning and processing of the child_pid variable/flag.
I also added some suggestions to make the user-facing information easier to grasp.

[erikmd]

Hi @AltGr @yurug, I believe I addressed all your suggestions.

[AltGr]

Nice, besides nitpicking on the manpage section the new option appears in, this seems all fine! :)

[erikmd]

@AltGr

while I agree that code-wise they are pretty different, from a user-perspective, they're both options concerning the server though (as the name implies!) -- and with similar use-cases so it would be best to have them closer to each other.

We could cheat by keeping this organisation in the code, but hardcoding ~docs to still put it in the SERVER section of the manpage (COMMON OPTIONS implicitely means the union of the commands, here we're dealing with the intersection :) )

Very good point, thanks! I fixed it as suggested.

Thanks again for your advice and help!

[erikmd]

With this last tweak (and rebase), CI is green.

@AltGr @yurug If you don't object, I will merge the PR today at 16:00 or so.