-
Notifications
You must be signed in to change notification settings - Fork 622
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
Restructure CI to use reusable steps, and add new checks #1921
base: main
Are you sure you want to change the base?
Restructure CI to use reusable steps, and add new checks #1921
Conversation
Two things to keep in mind for clang:
|
Good to know. I removed the Linux clang job just to limit the overall compute, but it sounds like we should leave at least one Linux clang in place. |
I think one test against the very latest mainline clang release we can get get our hands on is a good idea -- to guard against warnings that only appear in the latest clang but haven't migrated to Apple yet. |
This is a very exciting PR, by the way. I'm just waiting for free time to try this out on my approach projects' ci files as well. It looks like it can remove a lot of redundancy from the workflow files. |
Wow, I really mangled that typing. I meant "I'm just waiting for free time to try out this approach on my projects'..." |
106e1fb
to
c9fe762
Compare
99cc23b
to
079eeb0
Compare
Am I correct that adding the install_manifest files would need to be hand edited if a new header file/binary is added, or is there an automated process for building those that I missed? Perhaps a |
The goal of this change is to add tests to catch some recent regressions that the existing CI missed: * Validate that the proper files get installed. A recent innocuous-seeming change to the cmake config caused libdeflate files to make it into the installation and we didn't catch it. This change introduces a validation step that compares the install_mantifest.txt file from each job to a reference maintained in share/ci/install_manifest. Each job has a "build" number that identifies the reference manifest. NOTE: This means that all future changes to installed headers and libraries must be reflected in these manifests. * Validate building against prebuilt/external Imath and libdeflate libraries, and also test the "force internal" option. * Validate building with custom namespaces To make the workflow files easier to maintain, this splits two parts: * The ci_steps.yml file declares a common set of steps for Linux, macOS, and Windows, and a set of variables named to match the various CMake configuration options. * The ci_workflow.yml invokes these steps with a succession of builds that provide collections of CMake settings, which is passes to the reusable workflow via the "with:" statement. The "with:" statements provide default variable settings, which means each build specifies only non-default settings, for a compact summary of what's unique about that job. Some notes: * By default, the builds now pre-install Imath and libdeflate, so that the main OpenEXR build picks them up. A special build now validates the behavior of OPENEXR_FORCE_INTERNAL_IMATH and OPENEXR_FORCE_INTERNAL_DEFLATE, which were previously untested. * For each OS, we now build and test these configurations: 1. Release 2. Debug 3. Static 4. Threading disabled 5. pkgconfig, docs, examples, tools disabled. This was also previously not tested. 6. Legacy VFX reference platform compiler(s) and/or os(s) * The "label:" setting forms the description of the job on the GitHub Acions page, prepended with the OS and "build" number, which makes it easier to cross reference between the entries on the Actions page and the workflow file. * We no longer build with clang on Linux, since we do a clang build on macOS. Signed-off-by: Cary Phillips <[email protected]>
Signed-off-by: Cary Phillips <[email protected]>
@peterhillman, I thought about that and attempted something like it with a template that could match multiple builids, but I concluded it was too confusing since there are subtle differences between the naming of the installed files ( There are three scenarios:
|
079eeb0
to
ac6b58b
Compare
Fair point: the include system would simplify adding/removing a single file from the install but complicate everything else. |
Here's an example of failed run illustrating the error message, from my private cloned repo: https://github.com/cary-ilm/openexr/actions/runs/12126856323/job/33809845202 As an illustration, I removed The "Validate install" step fails and prints both manifests followed by this:
Maybe it's worth making that error message more elaborate, so it describes exactly what to do. It's coming from the validate_install.py script. You can also see an example of the uploaded artifacts here. It's a zip file that contains the install_manifest.txt files for each build. |
I added a more descriptive error message, it how reads like this:
The run is here: https://github.com/cary-ilm/openexr/actions/runs/12127537203/job/33812009971 |
The goal of this change is to add tests to catch some recent regressions that the existing CI missed:
Validate that the proper files get installed. A recent innocuous-seeming change to the cmake config caused libdeflate files to make it into the installation and we didn't catch it.
This change introduces a validation step that compares the install_mantifest.txt file from each job to a reference maintained in share/ci/install_manifest. Each job has a "build" number that identifies the reference manifest.
NOTE: This means that all future changes to installed headers and libraries must be reflected in these manifests.
Validate building against prebuilt/external Imath and libdeflate libraries, and also test the "force internal" option.
Validate building with custom namespaces
To make the workflow files easier to maintain, this splits two parts:
The ci_steps.yml file declares a common set of steps for Linux, macOS, and Windows, and a set of variables named to match the various CMake configuration options.
The ci_workflow.yml invokes these steps with a succession of builds that provide collections of CMake settings, which is passes to the reusable workflow via the "with:" statement. The "with:" statements provide default variable settings, which means each build specifies only non-default settings, for a compact summary of what's unique about that job.
Some notes:
By default, the builds now pre-install Imath and libdeflate, so that the main OpenEXR build picks them up. A special build now validates the behavior of OPENEXR_FORCE_INTERNAL_IMATH and OPENEXR_FORCE_INTERNAL_DEFLATE, which were previously untested.
For each OS, we now build and test these configurations:
The "label:" setting forms the description of the job on the GitHub Acions page, prepended with the OS and "build" number, which makes it easier to cross reference between the entries on the Actions page and the workflow file.
We no longer build with clang on Linux, since we do a clang build on macOS.
Website preview: https://openexr--1921.org.readthedocs.build/en/1921/