Add ci-windows-shared-local.yml #2309
Conversation
07ae2b8 to
02f3a7d
Compare
| -DAVIF_ENABLE_EXPERIMENTAL_YCGCO_R=ON | ||
| -DAVIF_ENABLE_EXPERIMENTAL_GAIN_MAP=ON | ||
| -DAVIF_ENABLE_EXPERIMENTAL_METAV1=ON | ||
| -DAVIF_ENABLE_EXPERIMENTAL_SAMPLE_TRANSFORM=ON |
There was a problem hiding this comment.
Just a thought: we could have a CI with a matrix testing all combinations of these experimental flags
There was a problem hiding this comment.
That's a lot of combinations (16) to test. I think we just need to test three combinations:
- No experimental features are enabled.
- Only the gain map experimental feature is enabled. (Chrome uses this combination.)
- All experimental features are enabled.
| - name: Prepare libavif (cmake) | ||
| run: > | ||
| cmake -G Ninja -S . -B build | ||
| -DCMAKE_BUILD_TYPE=Release -DBUILD_SHARED_LIBS=ON |
There was a problem hiding this comment.
Should Debug be tested too? I remember difficulties when linking differently built dependencies and wonder if that should be exercised. Probably not worth the effort.
There was a problem hiding this comment.
We can looking into adding Debug later. Right now I am focusing on getting a clean (no compiler or linker warnings) Release build.
There was a problem hiding this comment.
Yannis: While working on related build issues on Windows, I remembered the linking issue you were referring to.
In Debug build, we compile source files with /MDd on Windows. The lowercase d means we are using a debug version of the run-time library. All of our dependencies need to be compiled with /MDd to avoid a linker warning.
We may be able to set the CMAKE_MSVC_RUNTIME_LIBRARY variable so that we use the /MD compiler option in all build configurations.
|
Why not do a matrix in ci-windows.yml instead ? By adding |
| codec-aom: 'LOCAL' | ||
| codec-dav1d: 'LOCAL' | ||
| extra-cache-key: ${{ matrix.compiler }} | ||
| - name: Leave compiler as default |
There was a problem hiding this comment.
Setting those variables should probably be put in .github/actions/setup-windows
|
I am changing this pull request to a draft because there seem to be linker warnings, and there is an unresolved symbol linker error if experimental features are enabled. |
Add a CI workflow for building shared libraries with local dependencies on Windows.
Also use the TARGET_FILE generator expression.
46f69b0 to
4343faf
Compare
There was a problem hiding this comment.
I looked at the linker warnings carefully. I found that those linker warnings are only emitted when the linker error I reported in #2339 is there. So it should be fine to add this CI workflow as long as we avoid the linker error by turning off the gain map experimental feature. We can enable the other three experimental features.
| - name: Prepare libavif (cmake) | ||
| run: > | ||
| cmake -G Ninja -S . -B build | ||
| -DCMAKE_BUILD_TYPE=Release -DBUILD_SHARED_LIBS=ON |
There was a problem hiding this comment.
Yannis: While working on related build issues on Windows, I remembered the linking issue you were referring to.
In Debug build, we compile source files with /MDd on Windows. The lowercase d means we are using a debug version of the run-time library. All of our dependencies need to be compiled with /MDd to avoid a linker warning.
We may be able to set the CMAKE_MSVC_RUNTIME_LIBRARY variable so that we use the /MD compiler option in all build configurations.
| -DAVIF_ENABLE_EXPERIMENTAL_YCGCO_R=ON | ||
| -DAVIF_ENABLE_EXPERIMENTAL_GAIN_MAP=ON | ||
| -DAVIF_ENABLE_EXPERIMENTAL_METAV1=ON | ||
| -DAVIF_ENABLE_EXPERIMENTAL_SAMPLE_TRANSFORM=ON |
There was a problem hiding this comment.
That's a lot of combinations (16) to test. I think we just need to test three combinations:
- No experimental features are enabled.
- Only the gain map experimental feature is enabled. (Chrome uses this combination.)
- All experimental features are enabled.
Add a CI workflow for building shared libraries with local dependencies on Windows.