-
Notifications
You must be signed in to change notification settings - Fork 224
ci: add validation step for generated pkg-config files #1150
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
base: master
Are you sure you want to change the base?
Changes from 6 commits
5eb04c9
0de3e80
a7f6645
17c696a
71ddb0e
73eb5b6
e61c35e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,7 +5,6 @@ on: [push, pull_request] | |
| 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" | ||
| CTEST_TIME_TIMEOUT: "5" # some failures hang forever | ||
| HOMEBREW_NO_ANALYTICS: "ON" # Make Homebrew installation a little quicker | ||
| HOMEBREW_NO_AUTO_UPDATE: "ON" | ||
|
|
@@ -106,10 +105,47 @@ jobs: | |
| run: >- | ||
| ctest | ||
| --test-dir ${{ env.BUILD_DIR }} | ||
| --parallel | ||
| --output-on-failure | ||
| --no-tests=error | ||
|
|
||
| - name: Install project | ||
| if: ${{ contains(matrix.build, 'cmake') }} | ||
| run: cmake --install ${{ env.BUILD_DIR }} | ||
|
|
||
| - name: Validate generated pkg-config file | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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).
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| if: ${{ matrix.build == 'cmake' }} | ||
| shell: bash | ||
| run: | | ||
| # 1. Dynamically find the directory containing the .pc file (handles lib vs lib64) | ||
| PC_FILE=$(find $PWD/_dist -name "fortran_stdlib.pc" | head -n 1) | ||
|
|
||
| if [ -z "$PC_FILE" ]; then | ||
| echo "Error: fortran_stdlib.pc was not found in the installation directory!" | ||
| exit 1 | ||
| fi | ||
|
|
||
| PC_DIR=$(dirname "$PC_FILE") | ||
| export PKG_CONFIG_PATH="$PC_DIR:$PKG_CONFIG_PATH" | ||
| echo "Found pkg-config file in: $PC_DIR" | ||
|
|
||
| # 2. Check if the module can be found and the syntax is valid | ||
| pkg-config --validate fortran_stdlib | ||
|
|
||
| # 3. Extract the paths that pkg-config generated | ||
| LIB_DIR=$(pkg-config --variable=libdir fortran_stdlib) | ||
| INC_DIR=$(pkg-config --variable=includedir fortran_stdlib) | ||
|
|
||
| # 4. Verify the paths actually exist to catch double-prefixing | ||
| if [ ! -d "$LIB_DIR" ]; then | ||
| echo "Error: Library directory '$LIB_DIR' does not exist." | ||
| exit 1 | ||
| fi | ||
|
|
||
| if [ ! -d "$INC_DIR" ]; then | ||
| echo "Error: Include directory '$INC_DIR' does not exist." | ||
| exit 1 | ||
| fi | ||
|
|
||
| # 5. Print flags for debugging CI logs | ||
| echo "Resolved CFLAGS: $(pkg-config --cflags fortran_stdlib)" | ||
| echo "Resolved LIBS: $(pkg-config --libs fortran_stdlib)" | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @jvdp1 @jalvesz is removing this part will create any problem? I checked and tested and according to me it will not create any problem as such, that's why I suggest that it can be removed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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