-
Notifications
You must be signed in to change notification settings - Fork 16
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merge the CI workflows and stop using setup-ocaml
#473
Conversation
This is very nice, thanks a ton! 🙏 😃 Uniformity is appreciated in itself, but the current lack of it currently means that Linux, MinGW+Cygwin, and MSVC need three different changes to test a feature branch - and doing so for Cygwin is currently not possible without custom opam-files. On top, this PR eliminates the need for the (otherwise nice) Here are my initial impressions: To try this out on a concrete use case, I created a branch based on it targeting
https://github.com/ocaml-multicore/multicoretests/tree/shym-new-ci-common-winpthreadsectomy
One could of course target a feature branch by passing the appropriate repo and ref in each individual workflow, but it may be simpler to do so by 'overriding' and inputs-setting across the board as we can do now? 🤔 It would be good to update the current README instructions to reflect the new changes required. By chance I spotted that the Cygwin workflow is not running any multicoretests! (I realized because it completed before all other workflows and that has probably never happened before... 😅 ) |
Oh! Sorry about that! You’re right, fixed in e9fa0da (which I pushed to your branch, to trigger the intended tests), to squash into this PR, when I figure out why:
?!? |
I still don’t understand really why this is happening. In particular, I tried to launch Another outcome of investigating this is I realised that caching was not working on Cygwin (maybe on all Windows setups). This is hopefully fixed now. |
I just updated because it seems the terrible trick with pristine PATH is no longer necessary (it served to work around a bug of some binaries from Cygwin hiding the versions provided by the image when running the cache action; maybe we are no longer installing those binaries here, so it gets much simpler). And to fix a bug due to the fact that MSVC builds always need at least a script from OCaml sources, so we always fetch them in those runs. |
CI summary for 41d79d5
All of these are known and hence unrelated to the PR's changes. |
As it seems to be all good now, I’ve dropped the DROPME commit. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've now been over this.
This is a very carefully prepared change, that I really appreciate and that I'd like to see merged ASAP. Kudos for writing such a readable shell script!
I've made a few notes inline as I read this. The only actual issue is what I believe is a missed option.
A few high-level remarks:
- Should we go for installing named versions of dune and qcheck (e.g., 3.16 and 0.22)? That way accidental changes on either of their
main
branches won't affect a test suite outcome. The flip side is of course that these will need an occasional, manual version bump. - As for the workflow defaults, I'd rather just see an explicit
compiler_ref
mentioningtrunk
in the 5.4-trunk workflows (but that it is a matter of taste)
case "$OCAML_PLATFORM,$OCAML_OPTIONS" in | ||
msvc,*32bit*) | ||
eval $(tools/msvs-promote-path) | ||
printf 'Running: %s\n' "./configure --host=i686-pc-windows $opts" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps this is a combination (MSVC+32-bit) that is worth adding? (probably as a separate PR)
I deeply appreciate you keeping track of and supporting this, yet untested combination! 🙏
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It’s a combination that’s tested in ocaml/ocaml
CI, it helped iron out quite a few 64bit-isms while restoring MSVC 😅 I agree that it’d better be a separate PR.
I’ve just pushed on a new new-ci-common-2 branch all the fixes you suggested (the branch might be nicer to review the diff). I’ll let the CI round go through and rebase them in the PR branch. |
The patches on that branch look good to me! Before merging I just remembered this comment from above:
I was thinking of the last half of https://github.com/ocaml-multicore/multicoretests/blob/main/README.md#running-the-test-suite which could be changed to something like the following (feel free to reword/change as you see fit):
|
I also remembered it and I had pushed a commit to that effect on the new branch 😄 By the way, I went with the fully qualified name ( |
Merge the two CI main workflows, namely `common.yml` and `msvc-common.yml` into a single `common.yml` workflow that uses neither the `setup-ocaml` action nor `OPAM` so that: - the workflow has reduced dependencies and a smaller footprint, - the compiler configuration can be tuned (to disable features that are not relevant to the multicoretests, such as manpages), - it gets easier to test any branch of any fork of the compiler, - it brings the workflow really close to a version that can be pushed to the compiler repository. The logic of the workflow is largely extracted to a `runner.sh` script so that: - it is easier to maintain, as it is a regular script, instead of being embedded into a different syntax, especially with the issue of portability to Windows due to line endings, - it is easy to reuse that logic for different setups than regular GH Action runners, such as VMs, other infrastructures, etc. Extra notes: - the actual test steps are currently not triggered through the `runner.sh` script, in the unlikely case this could make some difference on Windows to have an `sh` involved somewhere in the stack, - the new common-workflow default is to test `trunk` rather than 5.0.0, which seems a better default.
CI round all green, I’ve force-pushed the new version. |
CI summary for 0a058f0: all 45 workflows succeeded! |
CI summary for merge to |
This PR merges the two CI main workflows, namely
common.yml
andmsvc-common.yml
into a singlecommon.yml
workflow that uses neither thesetup-ocaml
action norOPAM
so that:The logic of the workflow is largely extracted to a
runner.sh
script so that:Extra notes:
runner.sh
script, in the unlikely case this could make some difference on Windows to have ansh
involved somewhere in the stack,trunk
rather than 5.0.0, which seems a better default.Unfortunately, most of this PR is a single large commit that rewrites the common workflows and pulls out the
runner.sh
script. I didn’t find good intermediate steps for this rewrite.I’ve tested this PR by checking that the output of
ocamlc -config
is indeed the expected one in all runs. I hope I didn’t miss something on the way.Open question: I wondered whether we should use fixed versions of QCheck and dune rather than the latest commit. If I remember correctely, it is currently done so in the MSVC workflows as a couple of fixes were required from both projects to get the build working. Now that they have been released (at least in
dune
?), we could go back to the latest released versions.