Update GitHub Actions workflows #1782
Conversation
There was a problem hiding this comment.
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (2)
.github/workflows/build-wasm_of_ocaml.yml:20
- [nitpick] The version format for ocaml-compiler has changed from '4.14.x' to '4.14'. Ensure this change is intentional and compatible with the expected format.
- - 4.14.x
.github/workflows/build-wasm_of_ocaml.yml:32
- [nitpick] The version format for ocaml-compiler has changed from '4.14.x' to '4.14'. Ensure this change is intentional and compatible with the expected format.
- ocaml-compiler: 4.14.x
smorimoto
force-pushed
the
update-gha-workflow
branch
8 times, most recently
from
8b93bb6 to
b456f1e
Compare
Signed-off-by: Sora Morimoto <[email protected]>
smorimoto
force-pushed
the
update-gha-workflow
branch
from
b456f1e to
e24a802
Compare
There was a problem hiding this comment.
Copilot reviewed 4 out of 6 changed files in this pull request and generated no comments.
Files not reviewed (2)
- dune: Language not supported
- tools/ci_setup.ml: Language not supported
Comments suppressed due to low confidence (1)
.github/workflows/build.yml:86
- [nitpick] The condition might be too restrictive. Consider verifying if the condition should be more general to cover other operating systems or OCaml versions.
if: ${{ matrix.os == 'windows-latest' && matrix.ocaml-compiler < 5.2 }}
| dune-cache: true | ||
| opam-pin: false | ||
|
|
||
| - run: opam install conf-pkg-config |
There was a problem hiding this comment.
I think this is the second time you try to remove that line. It was here on purpose to work-around an opam packaging issue. Are you sure it is no longer needed
There was a problem hiding this comment.
Yes, it's no longer needed.
There was a problem hiding this comment.
To be more clear, zarith now explicitly requires it, and conf-pkg-config is work properly in the Cygwin environment.
There was a problem hiding this comment.
I don't remember this being related to zarith
There was a problem hiding this comment.
At least for now, zarith is the only library which requires conf-pkg-config (on Windows)
There was a problem hiding this comment.
I though the issue was with mingw-64-shims and its optional dep on conf-pkg-config. Here is an extract of a private discussion with @dra27
Oh, I see what's happening here - it's a race
There's an instant during the recompile where the old mingw-64-shims package has been removed (so there's no x86_64-w64-mingw32-gcc shim installed) and those packages then try to call the C compiler.
@dra27 do you remember this issue, can you confirm it was fixed ?
There was a problem hiding this comment.
If I remember correctly, it was fixed in mingw-w64-shims.0.2.0.
There was a problem hiding this comment.
No, I have failing logs involving mingw-w64-shims.0.2.0
There was a problem hiding this comment.
|
@smorimoto, I'm not very happy with how you push changes without explanation/description and review. |
|
I foolishly thought it was clear enough |
| separate_compilation: false | ||
|
|
||
| runs-on: ${{ matrix.os }} | ||
|
|
||
| steps: | ||
| - name: Set git to use LF |
There was a problem hiding this comment.
This will need to come back once we start testing windows
There was a problem hiding this comment.
In the upcoming PR, I'm going to make changes to separate the jobs for testing and installation testing so that we can run all tests on all platforms.
There was a problem hiding this comment.
separate the jobs for testing and installation testing
I don't understand what you mean by that.
At least the following deserves an explanation and is clearly not clear enough if you push it through without further comment. Can you point to GH issue tracking this ? I haven't seen any CI faliure related to this. |
| @@ -212,7 +212,7 @@ let pin delay nm = | |||
| (Printf.sprintf | |||
| "opam pin add -n %s https://github.com/ocaml-wasm/%s.git#wasm" | |||
| (try List.assoc nm aliases | |||
| with Not_found -> if List.mem_assoc nm packages then nm ^ ".v0.16.0" else nm) | |||
| with Not_found -> if List.mem_assoc nm packages then nm ^ ".v0.16.1" else nm) | |||
There was a problem hiding this comment.
Yet another change hidden in the noise that is not explained.
@smorimoto, Ping, can you explain the issue ? |
|
It seems some tests are no longer run: |
Attempted fix: #1789 |
Check the CI does not complain...