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

Fix configuration export #290

Merged
merged 5 commits into from
Aug 7, 2023
Merged

Fix configuration export #290

merged 5 commits into from
Aug 7, 2023

Conversation

mauneyc-LANL
Copy link
Collaborator

PR Summary

@jhp-lanl found an issue where the exported singularity-eosConfig.cmake didn't have guards to check if a target has already been created for dependencies. This would cause an issue with builds where there are common dependencies downstream.

This update includes those guards, and also simplifies the logic.

PR Checklist

  • Adds a test for any bugs fixed. Adds tests for new features.
  • Format your changes by using the make format command after configuring with cmake.
  • Document any new features, update documentation for changes made.
  • Make sure the copyright notice on any files you modified is up to date.
  • After creating a pull request, note it in the CHANGELOG.md file
  • If preparing for a new release, update the version in cmake.

Sorry, something went wrong.

@mauneyc-LANL mauneyc-LANL self-assigned this Aug 7, 2023
@mauneyc-LANL mauneyc-LANL changed the title Fix configuration export WIP: Fix configuration export Aug 7, 2023
@mauneyc-LANL mauneyc-LANL marked this pull request as draft August 7, 2023 20:35
@mauneyc-LANL mauneyc-LANL changed the title WIP: Fix configuration export Fix configuration export Aug 7, 2023
@jhp-lanl
Copy link
Collaborator

jhp-lanl commented Aug 7, 2023

It blows my mind that with how much boilerplate this has that there isn't already CMake functionality for this. And I don't want to block anything, but would it make sense for this to be a macro wrapping find_package()? I'm also fine with these changes as-is

@jhp-lanl
Copy link
Collaborator

jhp-lanl commented Aug 7, 2023

@mauneyc-LANL do we need to run the CI on gitlab as well?

@mauneyc-LANL mauneyc-LANL marked this pull request as ready for review August 7, 2023 20:57
@mauneyc-LANL
Copy link
Collaborator Author

It blows my mind that with how much boilerplate this has that there isn't already CMake functionality for this. And I don't want to block anything, but would it make sense for this to be a macro wrapping find_package()? I'm also fine with these changes as-is

Probably this should be used. I can do this for this PR or later

@mauneyc-LANL
Copy link
Collaborator Author

@mauneyc-LANL do we need to run the CI on gitlab as well?

There's no test for this on the CI; there's been some discussion on doing this and maybe should plan for it. Locally I just copy the examples folder and have a CMake with find_package(singularity-eos). That would be easy to get onto the CI i think

@mauneyc-LANL
Copy link
Collaborator Author

@jhp-lanl I've updated to use find_dependency (from comment above) that should avoid find_package collisions without doing a manual check (though if it still has issues then we can do manual checks).

@jhp-lanl
Copy link
Collaborator

jhp-lanl commented Aug 7, 2023

Okay the CMake configure stage seems to work now (which is where I was seeing the failure before). Merging

@jhp-lanl jhp-lanl merged commit 984e6d0 into main Aug 7, 2023
@jhp-lanl jhp-lanl deleted the mauneyc/fix-export-config branch August 7, 2023 23:21
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.

None yet

3 participants