GHA: add an optional wider test matrix (Cygwin, static, minimal, etc.)#14013
Merged
dra27 merged 12 commits intoocaml:trunkfrom May 5, 2025
Merged
GHA: add an optional wider test matrix (Cygwin, static, minimal, etc.)#14013dra27 merged 12 commits intoocaml:trunkfrom
dra27 merged 12 commits intoocaml:trunkfrom
Conversation
Member
Author
|
Example on my fork with the two labels applied - dra27#195 (I won’t add the new labels here until we’ve agreed both that we want to do it and on the names!) |
MisterDA
approved these changes
May 5, 2025
Contributor
There was a problem hiding this comment.
I like all these changes. I appreciate how the configure arguments are now handled, and the introduction of the config job. I made a minor suggestion on the C code.
I'm a bit puzzled by the double comments: why are // # needed in all these lines?
I'm tempted to suggest to switch to a list of arches in the build matrix: instead of having x86_64: [true, false], we could have arch: ['x86', 'x86_64'], which I find cleaner; and in the future arm64!
The label run-cygwin-tests was introduced in my previous attempt, it can be removed.
ocamltest/run_stubs.c
Outdated
Comment on lines
+140
to
+143
| int i = 0; | ||
| LUID_AND_ATTRIBUTES* privilege = process_privileges->Privileges; | ||
|
|
||
| while (i++ < process_privileges->PrivilegeCount) { |
Contributor
There was a problem hiding this comment.
Suggested change
| int i = 0; | |
| LUID_AND_ATTRIBUTES* privilege = process_privileges->Privileges; | |
| while (i++ < process_privileges->PrivilegeCount) { | |
| LUID_AND_ATTRIBUTES* privilege = process_privileges->Privileges; | |
| for (int i = 0; i < process_privileges->PrivilegeCount; i++) { |
Is there a reason you didn't opt for the for loop?
Member
Author
There was a problem hiding this comment.
Hah - because it was adapted from this in #8797:
/* Search for SeRestorePrivilege */
while (i < privs->PrivilegeCount
&& (privilege->Luid.HighPart != privilege_luid.HighPart
|| privilege->Luid.LowPart != privilege_luid.LowPart)) {
i++;
privilege++;
}(I can't resist the temptation to say "Luxury! When I were a lad we didn't have any of these 'ere for loops!")
Member
Author
Ah, that's because my editor doesn't recognise TypeScript embedded in YAML! 🫣 |
It is needed on Cygwin.
Define the matrix for the others job using an actions/github-script step, which allows the possibility of the matrix being dynamic.
Tests a full `--disable-shared` build on Linux and also a Linux build with as many options disabled as possible (as the minimal build in the other-configs job on Jenkins also does). The matrix is expanded by adding the 'CI: Full matrix' label to a pull request.
If Cygwin is running "elevated" - which it is in CI - then it acts as though it's running as root. It intentionally activates SeBackupPrivilege, which thwarts the test_create_cursor_failures.ml test. The OCaml testsuite will never require root privileges for anything meaningful, so ocamltest on Cygwin simply drops the SeBackupPrivilege when running on Cygwin, which means the test correctly fails.
Member
Author
|
I pushed an extra commit trying out the |
MisterDA
approved these changes
May 5, 2025
Yes, yes, the filename -msvc.yml has become a bit out of date...
Applying this label _before_ the testsuite has started in either GitHub Actions or AppVeyor will cause the execution of the testsuite to be skipped.
Member
Author
|
I've renamed the |
Member
|
Is it expected for one of the GHA jobs to be named EDIT: ah nevermind, it's in the "ThreadSanitizer" sub-category |
Member
Author
|
That’s something I hope to deal with when they all get merged - the semantics of an empty or a skipped matrix are a bit meh! |
dra27
added a commit
to dra27/ocaml
that referenced
this pull request
May 19, 2025
GHA: add an optional wider test matrix (Cygwin, static, minimal, etc.) (cherry picked from commit 96b56fd)
dra27
added a commit
to dra27/ocaml
that referenced
this pull request
May 19, 2025
GHA: add an optional wider test matrix (Cygwin, static, minimal, etc.) (cherry picked from commit 96b56fd)
dra27
added a commit
to dra27/ocaml
that referenced
this pull request
May 19, 2025
GHA: add an optional wider test matrix (Cygwin, static, minimal, etc.) (cherry picked from commit 96b56fd)
dra27
added a commit
to dra27/ocaml
that referenced
this pull request
May 19, 2025
GHA: add an optional wider test matrix (Cygwin, static, minimal, etc.) (cherry picked from commit 96b56fd)
dra27
added a commit
to dra27/ocaml
that referenced
this pull request
May 19, 2025
GHA: add an optional wider test matrix (Cygwin, static, minimal, etc.) (cherry picked from commit 96b56fd)
dra27
added a commit
to dra27/ocaml
that referenced
this pull request
May 19, 2025
GHA: add an optional wider test matrix (Cygwin, static, minimal, etc.) (cherry picked from commit 96b56fd)
dra27
added a commit
to dra27/ocaml
that referenced
this pull request
May 19, 2025
GHA: add an optional wider test matrix (Cygwin, static, minimal, etc.) (cherry picked from commit 96b56fd)
dra27
added a commit
to dra27/ocaml
that referenced
this pull request
May 19, 2025
GHA: add an optional wider test matrix (Cygwin, static, minimal, etc.) (cherry picked from commit 96b56fd)
dra27
added a commit
to dra27/ocaml
that referenced
this pull request
May 19, 2025
GHA: add an optional wider test matrix (Cygwin, static, minimal, etc.) (cherry picked from commit 96b56fd)
dra27
added a commit
to dra27/ocaml
that referenced
this pull request
May 19, 2025
GHA: add an optional wider test matrix (Cygwin, static, minimal, etc.) (cherry picked from commit 96b56fd)
dra27
added a commit
to dra27/ocaml
that referenced
this pull request
May 19, 2025
GHA: add an optional wider test matrix (Cygwin, static, minimal, etc.) (cherry picked from commit 96b56fd)
dra27
added a commit
to dra27/ocaml
that referenced
this pull request
May 19, 2025
GHA: add an optional wider test matrix (Cygwin, static, minimal, etc.) (cherry picked from commit 96b56fd)
dra27
added a commit
that referenced
this pull request
May 19, 2025
GHA: add an optional wider test matrix (Cygwin, static, minimal, etc.) (cherry picked from commit 96b56fd)
dra27
added a commit
to dra27/ocaml
that referenced
this pull request
May 20, 2025
GHA: add an optional wider test matrix (Cygwin, static, minimal, etc.) (cherry picked from commit 96b56fd)
dra27
added a commit
to dra27/ocaml
that referenced
this pull request
May 20, 2025
GHA: add an optional wider test matrix (Cygwin, static, minimal, etc.) (cherry picked from commit 96b56fd)
dra27
added a commit
to dra27/ocaml
that referenced
this pull request
May 20, 2025
GHA: add an optional wider test matrix (Cygwin, static, minimal, etc.) (cherry picked from commit 96b56fd)
dra27
added a commit
that referenced
this pull request
May 20, 2025
GHA: add an optional wider test matrix (Cygwin, static, minimal, etc.) (cherry picked from commit 96b56fd)
dra27
added a commit
to dra27/ocaml
that referenced
this pull request
May 20, 2025
GHA: add an optional wider test matrix (Cygwin, static, minimal, etc.) (cherry picked from commit 96b56fd)
dra27
added a commit
to dra27/ocaml
that referenced
this pull request
May 20, 2025
GHA: add an optional wider test matrix (Cygwin, static, minimal, etc.) (cherry picked from commit 96b56fd)
dra27
added a commit
to dra27/ocaml
that referenced
this pull request
May 20, 2025
GHA: add an optional wider test matrix (Cygwin, static, minimal, etc.) (cherry picked from commit 96b56fd)
dra27
added a commit
to dra27/ocaml
that referenced
this pull request
May 20, 2025
GHA: add an optional wider test matrix (Cygwin, static, minimal, etc.) (cherry picked from commit 96b56fd)
dra27
added a commit
to dra27/ocaml
that referenced
this pull request
Jun 27, 2025
GHA: add an optional wider test matrix (Cygwin, static, minimal, etc.) (cherry picked from commit 96b56fd)
dra27
added a commit
to dra27/ocaml
that referenced
this pull request
Jun 27, 2025
GHA: add an optional wider test matrix (Cygwin, static, minimal, etc.) (cherry picked from commit 96b56fd)
This was referenced Sep 13, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This adds a couple of things I've been using over on my fork for Relocatable OCaml for the last few months:
CI: Full matrixlabel--disable-sharedjob on LinuxCI: Skip testsuitelabelI've pulled in and updated the documentation improvements from #13996 and also the fix to
tools/check-symbol-namesas my version hadn't originally been calling that. Unlike in #13996, I opted to fix thetest_create_cursor_failures.mltest, as the failure was for the same reason as the i386 run originally in #13419. To do, I've adapted a fix in the style of #8797 inocamltestonly.I've included comments in the YAML for the part which generates the matrix, but note that the workflow here is to avoid re-running either of the build workflows automatically if the labels are added/removed - the idea is to re-run the config job in each workflow manually if the labels have been changed. There's more merging work with these workflows to do in the future, but I think this is a reasonable step forward...