Skip to content

pkg confg#497

Merged
Matthew Hambley (MatthewHambley) merged 22 commits into
mainfrom
418_pkg_confg
Jun 3, 2026
Merged

pkg confg#497
Matthew Hambley (MatthewHambley) merged 22 commits into
mainfrom
418_pkg_confg

Conversation

@hiker
Copy link
Copy Markdown
Collaborator

@hiker Joerg Henrichs (hiker) commented Sep 15, 2025

Adds a simple interface to pkg-config and nf-config.

Closes #418

@hiker
Copy link
Copy Markdown
Collaborator Author

Yaswant Pradhan (@yaswant) , ready for review. Not urgent for the release, it's just makes site-configs more convenient to write.

@hiker Joerg Henrichs (hiker) added this to the vn2.1 milestone Oct 6, 2025
@hiker Joerg Henrichs (hiker) added the Ready for review Indicating that a PR is ready to be reviewed. label Oct 23, 2025
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure this is a problem which needs an object solution. In particular an object per library seems a bit odd since that obvious is immediately thrown away after the library has been interrogated.

It may be helpful to look at https://github.com/MetOffice/lfric_core/blob/main/lfric_build/pkg_config.py for an alternative approach. Although this is not necessarily the best solution either.

Also, the shell command seems like a different change. Can it be moved to its own pull request?

Comment thread tests/unit_tests/tools/test_pkg_config.py
@hiker
Copy link
Copy Markdown
Collaborator Author

I'm not sure this is a problem which needs an object solution. In particular an object per library seems a bit odd since that obvious is immediately thrown away after the library has been interrogated.

That is the first time that I got a comment to remove OO ;) The classes here inherit from the Tool class the handling of availability, version number, and code execution and error handling. Why would we want to remove this feature of OO, and potentially duplicate the code here?

It may be helpful to look at https://github.com/MetOffice/lfric_core/blob/main/lfric_build/pkg_config.py for an alternative approach. Although this is not necessarily the best solution either.

I had a quick look, and while it supports version handling and better shared/static support, it's otherwise pretty much the same - one class per package? I don't see the real difference to the way this implementation is handling this.

Also, after looking at the code, it appears that it combined -L, -I etc and the path into a single argument. I am not really familiar with pkg-config, why is that required?

But the support for version specification and better static/shared handling would actually be good to add imho.

Also, the shell command seems like a different change. Can it be moved to its own pull request?

I added a new section to the documentation to document the new tools, and then added a paragraph of explicit documentation for the existing shell tool to this new section to complete it. Should I really pull out one paragraph of documentation to a separate PR?

I wonder if perhaps the previous PR wasn't clean or so, and showed some other changes? I've just updated this PR to latest main version (and added the two new tools to fab.api), and it now does not show any changes to shell.py.

TBH, since your implementation has already support for version specifications and static/shared linking, I am also happy to close this PR, so you can add your version instead.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One minor issue of location.

Comment thread source/fab/tools/nf_config.py Outdated
@hiker
Copy link
Copy Markdown
Collaborator Author

I had to merge #569 in (since pkg config used the MISC category. Given that #569 is tiny, I think that's ok, and I'll close the separate PR.

@hiker Joerg Henrichs (hiker) added Priority Indicate an issue that should be done in the near future. and removed Low priority labels May 28, 2026
@hiker
Copy link
Copy Markdown
Collaborator Author

Sam Clarke-Green (@t00sa) , this PR is now required for current lfric_core (while we don't need pkg_config support necessarily, this PR re-adds Category.MISC, and several tools in lfric needs this). So, I am marking this as high priority. Not sure what milestone to assign, but it would be great to have this for the next lfric release.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets get this change on.

@MatthewHambley Matthew Hambley (MatthewHambley) changed the title 418 pkg confg 418 pkg confg WIP Jun 3, 2026
@MatthewHambley Matthew Hambley (MatthewHambley) changed the title 418 pkg confg WIP pkg confg Jun 3, 2026
@MatthewHambley Matthew Hambley (MatthewHambley) merged commit 41ebc4c into main Jun 3, 2026
8 checks passed
@MatthewHambley Matthew Hambley (MatthewHambley) deleted the 418_pkg_confg branch June 3, 2026 10:54
@github-project-automation github-project-automation Bot moved this from In Review to Complete in Fab Development Tracker Jun 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Priority Indicate an issue that should be done in the near future. Ready for review Indicating that a PR is ready to be reviewed.

Projects

Status: Complete

Development

Successfully merging this pull request may close these issues.

Add support for pkg-config files

4 participants