ci: add validation step for generated pkg-config files#1150
ci: add validation step for generated pkg-config files#1150srinjoy933 wants to merge 7 commits intofortran-lang:masterfrom
Conversation
jvdp1
left a comment
jvdp1
left a comment
There was a problem hiding this comment.
Thank you @srinjoy933 . I left a few comments.
.github/workflows/CI.yml
Outdated
| if: ${{ contains(matrix.build, 'cmake') }} | ||
| shell: bash | ||
| run: | | ||
| # Ensure pkg-config is available |
There was a problem hiding this comment.
Is there not "action" defined somewhere to setup pkg-config?
It would help to avoid these if-else conditions
There was a problem hiding this comment.
Hi @jvdp1, thank you for the review! It turns out pkg-config is pre-installed on the default GitHub Ubuntu and macOS runners, so I have completely removed the if-else installation block to keep the workflow clean.
.github/workflows/CI.yml
Outdated
| pkg-config --validate fortran_stdlib | ||
|
|
||
| # Test flags to ensure no errors are thrown | ||
| pkg-config --cflags --libs fortran_stdlib No newline at end of file |
There was a problem hiding this comment.
If I am correct, this will return the paths, but it won't check if the paths are correct. what is the expectation?
There was a problem hiding this comment.
You are correct that --cflags --libs only prints the paths but doesn't verify them. To actually validate the paths (and catch any double-prefixing bugs), I've updated the script to extract the libdir and includedir variables from pkg-config and explicitly verify that those directories exist on the filesystem.
There was a problem hiding this comment.
Pull request overview
Adds a CI safeguard to prevent regressions in the generated pkg-config export produced by the CMake install, aligning with the goal in issue #1149 to automatically validate the .pc output in the Linux/macOS CMake workflow matrix.
Changes:
- Adds a post-install validation step that ensures
pkg-configis available on the runner. - Sets
PKG_CONFIG_PATHto the local install prefix ($PWD/_dist) so validation targets the just-installed.pcfile. - Runs
pkg-config --validateplus a--cflags/--libsresolution check forfortran_stdlib.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1150 +/- ##
=======================================
Coverage 68.66% 68.66%
=======================================
Files 408 408
Lines 13619 13619
Branches 1537 1537
=======================================
Hits 9351 9351
Misses 4268 4268 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@jvdp1 I have made the changes according to your suggestion .Please have a look at this now, if it is ok or not. |
…add-pkg-config-ci
| env: | ||
| CMAKE_BUILD_PARALLEL_LEVEL: "2" # 2 cores on each GHA VM, enable parallel builds | ||
| CTEST_OUTPUT_ON_FAILURE: "ON" # This way we don't need a flag to ctest | ||
| CTEST_PARALLEL_LEVEL: "2" |
There was a problem hiding this comment.
Hi @jvdp1! I removed the parallel testing flags to bypass a race condition in the upstream test suite that was causing the CI to crash on this branch.Specifically, the I/O example tests (example_open, example_savetxt, and example_loadtxt) all hardcode their read/write operations to the exact same file (example/io/example.dat). When CTest runs them in parallel, they step on each other (e.g., loadtxt tries to read floats while example_open is writing English text to the same file), which causes an .
Removing the parallel flag forces CTest to run them sequentially, which immediately fixed the CI.
There was a problem hiding this comment.
I never like to remove a feature (here parallelization) to solve another problem (here with I/O tests). It will only hide the underlying problem that will re-appear later. Therefore, I suggest to add the parallelization feature and try to solve the underlying problem in another PR
| if: ${{ contains(matrix.build, 'cmake') }} | ||
| run: cmake --install ${{ env.BUILD_DIR }} | ||
|
|
||
| - name: Validate generated pkg-config file |
There was a problem hiding this comment.
Thank you @srinjoy933 . It is a nice improvment, and the paths are now checked.
In the past, we got some issues with the pkg-config files (e..g, issues with the order of the libs, ...; see e.g. #1102).
I wonder if it would be approriate to add a specific CI yml file for testing pkg-config files (even for compilation).
There was a problem hiding this comment.
Thank you @jvdp1 ! I completely agree. Setting up a dedicated .yml workflow that actually compiles a dummy Fortran program using the generated pkg-config flags is the best way to catch those linking order bugs like #1102.
Since this current PR is stable and successfully catches the basic path/double-prefixing errors, would you prefer to merge this ? I would be more than happy to open a fresh issue and a follow-up PR dedicated specifically to building out that standalone pkg-config compilation CI workflow.
|
hey @jvdp1 any suggestions and update regarding this pr? |
|
hey @jvdp1 as you have suggested I reverted the parallelization feature and will create another pr to solve the test problem. Please review this pr once . Thanks! |
This pull request introduces an automated validation step within the CMake CI workflow (.github/workflows/CI.yml) to ensure the structural integrity of generated pkg-config exports. Building on recent fixes for absolute pathing in .pc files, this addition prevents future regressions by strictly testing the generated fortran_stdlib.pc file immediately following the cmake --install step. The workflow now ensures pkg-config is present on the runner, maps the PKG_CONFIG_PATH to the local _dist installation directory, and executes both a strict --validate check and a test of the --cflags and --libs flag resolution.
Solve #1149