Skip to content
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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

cary-ilm
Copy link
Member

@cary-ilm cary-ilm commented Nov 19, 2024

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.

Website preview: https://openexr--1921.org.readthedocs.build/en/1921/

@lgritz
Copy link
Contributor

lgritz commented Nov 20, 2024

Two things to keep in mind for clang:

  1. The clang on MacOS is Apple Clang, which both lags behind and doesn't quite match any released version of generic clang that you would be getting on Linux.
  2. On Mac, libc++ (llvm's std library) is used, whereas on linux when you use clang, it's still using gcc's libstdc++ by default (you can force clang to use libc++ on any platform, but it's not ABI compatible with libstdc++, so you can't build one library in isolation with libc++).

@cary-ilm
Copy link
Member Author

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.

@lgritz
Copy link
Contributor

lgritz commented Nov 20, 2024

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.

@lgritz
Copy link
Contributor

lgritz commented Nov 20, 2024

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.

@lgritz
Copy link
Contributor

lgritz commented Nov 20, 2024

Wow, I really mangled that typing. I meant "I'm just waiting for free time to try out this approach on my projects'..."

@peterhillman
Copy link
Contributor

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 #include sort of mechanism in the manifest files would make things less error prone? That way, only one file needs to be updated when a header file is added, since that file would be referenced by the other manifests as needed. Adding a new binary might mean updating two files: one for windows which lists binaries with a .exe extension and one for linux and macos

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]>
@cary-ilm
Copy link
Member Author

cary-ilm commented Dec 2, 2024

@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 (_d for Debug ibs, etc), and other idiosyncrasies of each build. I did add variables that expand to the major.minor.patch release numbers, so they don't have to be updated. Changing what's installed happened so seldom that I think a small bit of straightforward tedium is better than cleverness you can never quite remember.

There are three scenarios:

  1. Adding/removing/renaming a single file. It will be tedious to touch a bunch of files when adding/removing an installed header/binary, but at least it's straightforward what to do.

  2. A major reshuffle. The install manifests are uploaded as build artifacts, so if a lot changes, just download the artifacts and commit them.

  3. Adding a new build. Submit the build, let the validation step fail, download the install manifest artifact, and commit it.

@peterhillman
Copy link
Contributor

Fair point: the include system would simplify adding/removing a single file from the install but complicate everything else.
I wonder how easy it will be to find the error messages reported by validate_install when it causes a build failure, and if it'll be apparent enough what to do to solve the problems it finds?

@cary-ilm
Copy link
Member Author

cary-ilm commented Dec 2, 2024

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 ImfAcesFile.h as a header in the CMakeLists.txt and added ImfFoo.h just to show what it looks like.

The "Validate install" step fails and prints both manifests followed by this:

Error: The following files should have been installed but weren't:
  include/OpenEXR/ImfAcesFile.h
Error: The following files were installed but were not expected:
  include/OpenEXR/ImfFoo.h
Error: Process completed with exit code 1.

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.

@cary-ilm
Copy link
Member Author

cary-ilm commented Dec 2, 2024

I added a more descriptive error message, it how reads like this:

Error: The following files should have been installed but weren't:
  include/OpenEXR/ImfAcesFile.h
If these files have been intentionally deprecated and should no longer be installed via 'make install',
then you should remove references to them in the archived install manifests in share/ci/install_manifest/install_manifest.*.*.txt.
If they should be installed, something has gone wrong with the project CMake configuration and should be fixed.
Or possibly the expected behavior of this CI build has changed in the workflow file, so you should edit the corresponding archived install manifest to bring it in line with the expected output.
Error: The following files were installed but were not expected:
  include/OpenEXR/ImfFoo.h
If these are new files introduced to the project and should now be installed,
then you should add references to them in the archived install manifests in share/ci/install_manifest/install_manifest.*.*.txt.
Or possibly the expected behavior of this CI build has changed in the workflow file, so you should edit the corresponding archived install manifest to bring it in line with the expected output.
Error: Process completed with exit code 1.

The run is here: https://github.com/cary-ilm/openexr/actions/runs/12127537203/job/33812009971

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants