Fix learn-ocaml-client regression after PR #320

[erikmd]

With @LouisAyroles and @Fixiss, we noticed that there were some (unexpected) incompatibility between learn-ocaml-client (say, from master) and the latest release of learn-ocaml (0.12).

Here a minimal working example to reproduce the bug:

cd .../learn-ocaml
docker run --name=locaml -d -it -p 8080:8080 -v "$PWD/demo-repository:/repository" ocamlsf/learn-ocaml:0.12
TOKEN=$(docker logs locaml | grep -e 'Initial teacher token created:' | sed -e 's/^.*: //')
# then
git checkout master
opam install . --deps-only --locked -y
opam install -y opam-installer.$(opam config var opam-version)
eval $(opam env)
make
make opaminstall
learn-ocaml-client init -s http://localhost:8080 -t "$TOKEN"
touch demo.ml
learn-ocaml-client grade demo.ml

then we were getting:

Fatal error: exception Json_encoding.Cannot_destruct(_)
Called from unknown location
Called from unknown location
Called from unknown location
Called from unknown location
Called from unknown location
Called from unknown location
Called from unknown location

Thanks to git bisect and further explorations, it appears the issue was just introduced in PR #320.

This new PR attempts to fix this issue (very important to have some smooth experience with learn-ocaml-client and learn-ocaml.el), and I'll proceed in a TDD-like manner (extended the GHA CI test-suite within this PR).

[erikmd]

This PR is now ready, but before merging I'd like to have some confirmation by @yurug that the fix-commit cd9291e does not have unforeseen side-effects, on existing exercises you may have prepared, and that would specifically rely on the depend.txt feature.

Summary of the PR

• The first commit exhibits the issue, as displayed in this GHA log:

  Run cd tests && bash -c ./runtests.sh
  Running tests with BOTH images (learn-ocaml, learn-ocaml-client)
  ---> Entering ./test1:
  Configuration written to /home/learn-ocaml/.config/learnocaml/client.json.
  NOT OK: ./test1/demo2.ml
  Reading solution...
  Fetching exercise data from server...
  Fatal error: exception Json_encoding.Cannot_destruct(_)
  Called from unknown location
  Called from unknown location
  Called from unknown location
  Called from unknown location
  Called from unknown location
  Called from unknown location
  Called from unknown location
  Error: Process completed with exit code 1.
  

• The second commit fixes the compatibility issue, as displayed in this GHA log:

  Run cd tests && bash -c ./runtests.sh
  Running tests with BOTH images (learn-ocaml, learn-ocaml-client)
  ---> Entering ./test1:
  Configuration written to /home/learn-ocaml/.config/learnocaml/client.json.
  OK: ./test1/demo2.ml
  OK: ./test1/bad_plus_2.ml
  OK: ./test1/demo.ml
  OK: ./test1/bad_plus.ml
  find: ‘./corpuses’: No such file or directory
  

  where (learn-ocaml, learn-ocaml-client) are specifically chosen of version (HEAD, 0.12):

  Run docker inspect -f '{{json .Config.Labels}}' learn-ocaml-client learn-ocaml | jq
  {
    "org.opencontainers.image.description": "learn-ocaml command-line client",
    "org.opencontainers.image.title": "learn-ocaml-client",
    "org.opencontainers.image.url": "https://ocaml-sf.org/",
    "org.opencontainers.image.vendor": "The OCaml Software Foundation"
  }
  {
    "org.label-schema.build-date": "2020-02-19T20:59:22Z",
    "org.label-schema.description": "learn-ocaml app manager",
    "org.label-schema.name": "learn-ocaml",
    "org.label-schema.schema-version": "1.0",
    "org.label-schema.url": "https://ocaml-sf.org/",
    "org.label-schema.vcs-ref": "c54d3bb",
    "org.label-schema.vcs-url": "https://github.com/ocaml-sf/learn-ocaml",
    "org.label-schema.vendor": "The OCaml Software Foundation",
    "org.label-schema.version": "0.12"
  }
  

(Note that the GHA logs are not archived indefinitely, hence the copy-and-paste in the above snippets.)

[erikmd]

The last-but-one CI run failed because it seems the token is not yet created at the time the server listens to the TCP port…

I've pushed a slightly different way to get this token but I believe it might still fail.

FTR:

id=$(docker run --rm -d -it -p 8080:8080 -v "$PWD:/repository" ocamlsf/learn-ocaml:0.12)
docker logs -t $id
2021-06-23T12:52:53.899527427Z Learnocaml v.0.12 running.
2021-06-23T12:52:53.899557308Z Updating app at ./www
2021-06-23T12:52:53.943510782Z Learnocaml server v.0.12 starting on port 8080
2021-06-23T12:52:53.944241170Z Initial teacher token created: X-AYC-4FE-R1O-ZLD

so @yurug, wouldn't you mind if I rather write
./wait-for-it.sh localhost:8080 -s -t 10 -- sleep 1s?

this shouldn't be fragile (as unrelated to the build time of the repo): this extra second should just be required to account for the difference time _:53.944241170Z - _:53.943510782Z…

[yurug]

so @yurug, wouldn't you mind if I rather write
./wait-for-it.sh localhost:8080 -s -t 10 -- sleep 1s?

Fine for me!

[erikmd]

Thanks @yurug, actually wait-for-it.sh was not enough, so I needed to implement a small curl-based custom function.

Now the code related to your initial comment reads:

if ! wait_for_it "http://localhost:8080/version" "$build_timeout" sleep 1s  # ...

I thoroughly improved the bash script along the way, and did several tests locally: everything looks fine… so let's wait for the CI :)

[erikmd]

OK so if this very last CI run is OK ✔️, you'll be able to merge this PR anytime this Thursday, @yurug :)

[yurug]

Thank you @erikmd!

[erikmd]

Hi @yurug, small remark: it happens there are 3 PR merging strategies in GitHub (Merge, Merge/Squash, Merge/Rebase).

But Merge/Rebase has two drawbacks:

• there is no mention anywhere of the PR number in the resulting commits;
• and the resulting commits are not GPG-signed…

While Merge and Merge/Squash are very fine (Merge/Squash being especially fine if the PR contains only one commit).

So, maybe for upcoming PRs we should only use Merge or Merge/Squash 😅

[yurug]

OK. We indeed need to follow one merging strategy. Merge/Rebase has indeed these two drawbacks but also has advantages (more readable linear history is sometimes very useful). Anyway, your arguments are valid and I will use Merge and Merge/Squash in the future.