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

Checks for found and target in singularity-eosConfig.cmake #183

Merged
merged 3 commits into from
Oct 12, 2022

Conversation

mauneyc-LANL
Copy link
Collaborator

@mauneyc-LANL mauneyc-LANL commented Sep 28, 2022

PR Summary

The exported configuration checked for Eigen3_FOUND and passed, but the concurrent processing wasn't able to find the interface target Eigen3::Eigen. This updates the configuration check to check both.

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

@mauneyc-LANL mauneyc-LANL added the build Something to do with the build system label Sep 28, 2022
@mauneyc-LANL mauneyc-LANL self-assigned this Sep 28, 2022
Copy link
Collaborator

@Yurlungur Yurlungur left a comment

Choose a reason for hiding this comment

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

I don't understand these changes or their impact. But I'll trust you that they're correct. I'll approve once the boxes are all checked.

@mauneyc-LANL
Copy link
Collaborator Author

I don't understand these changes or their impact. But I'll trust you that they're correct. I'll approve once the boxes are all checked.

In the singularity-eosConfig.cmake file, what was happening was

if(NOT Eigen3_FOUND)
        find_package(Eigen3  REQUIRED)
endif()

What was happening when building downstream was Eigen3_FOUND was set by an earlier part of the build, but for some reason the target Eigen3::Eigen was not finding it's was down to singularity-eos. That is our target singularity-eos::singlarity-eos had an link dependency on Eigen3::Eigen, but at the end of the configuration step, CMake was complaining that singularity-eos depends on target Eigen3::Eigen, but that target was not found.

This is a bit brutish, but now singularity-eosConfig.cmake will have

if(NOT Eigen3_FOUND OR NOT TARGET Eigen3::Eigen)
        find_package(Eigen3  REQUIRED)
endif()

So that, if Eigen3::Eigen isn't a defined target, CMake will engage the find_package() tools to explicitly make it.

@dholladay00
Copy link
Collaborator

If I am understanding correctly, this change will be in effect for all dependencies @mauneyc-LANL? It seems like a good addition if we want to maintain in-tree and externally built capabilities for our dependencies.

@mauneyc-LANL
Copy link
Collaborator Author

mauneyc-LANL commented Oct 3, 2022

If I am understanding correctly, this change will be in effect for all dependencies @mauneyc-LANL? It seems like a good addition if we want to maintain in-tree and externally built capabilities for our dependencies.

This effects any dependencies that define an interface target that we adhere to, e.g. Eigen3::Eigen, ports-of-call; and, this is a fix for downstream codes that use find_package(singularity-eos), so any codes that manually add within a source-tree are not effected.

A counter-example is when we import HDF5, we no longer rely on an interface target and add the paths explicitly to the export config. This is b/c HDF5 doesn't have a consistent interface target for a range of cmake & hdf5 installs - that is, HDF5::HDF5 could resolve to different configurations downstream (or not be defined at all), and I threw my hands up and just rely on exporting explicit paths.

I should stress that this is a sloppy attempt to fix an issue I haven't figured out - specifically, when I was importing singularity-eos into a host code, that Eigen3_FOUND was set, but CMake was complaining that Eigen3::Eigen was not a defined target. This is likely due to some conflict in how a separate dependency was importing Eigen, but I didn't try and track that down. This change will ensure that the target Eigen3::Eigen is set by find_package(), regardless of what prior configuration has done.

@Yurlungur Yurlungur merged commit 38d1fc8 into main Oct 12, 2022
@Yurlungur Yurlungur deleted the mauneyc/fix/eigen-export-config branch October 12, 2022 21:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Something to do with the build system
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants