-
Notifications
You must be signed in to change notification settings - Fork 225
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 1 commit
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 |
|---|---|---|
|
|
@@ -113,3 +113,25 @@ jobs: | |
| - name: Install project | ||
| if: ${{ contains(matrix.build, 'cmake') }} | ||
| run: cmake --install ${{ env.BUILD_DIR }} | ||
|
|
||
| - name: Validate generated pkg-config file | ||
| if: ${{ contains(matrix.build, 'cmake') }} | ||
| shell: bash | ||
| run: | | ||
| # Ensure pkg-config is available | ||
|
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. Is there not "action" defined somewhere to setup
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. 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. |
||
| if ! command -v pkg-config &> /dev/null; then | ||
| if [ "$RUNNER_OS" == "Linux" ]; then | ||
| sudo apt-get update && sudo apt-get install -y pkg-config | ||
| elif [ "$RUNNER_OS" == "macOS" ]; then | ||
| brew install pkg-config | ||
| fi | ||
| fi | ||
|
|
||
| # Point to the installation directory specified by CMAKE_INSTALL_PREFIX | ||
| export PKG_CONFIG_PATH="$PWD/_dist/lib/pkgconfig:$PWD/_dist/share/pkgconfig:$PKG_CONFIG_PATH" | ||
|
|
||
| # Run the validation | ||
| pkg-config --validate fortran_stdlib | ||
|
|
||
| # Test flags to ensure no errors are thrown | ||
| pkg-config --cflags --libs fortran_stdlib | ||
|
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. If I am correct, this will return the paths, but it won't check if the paths are correct. what is the expectation?
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. 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.
Choose a reason for hiding this comment
The 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).
I wonder if it would be approriate to add a specific CI yml file for testing pkg-config files (even for compilation).
Uh oh!
There was an error while loading. Please reload this page.
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.
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.