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

git submodule fixes #561

Merged
merged 1 commit into from
Oct 21, 2022
Merged

git submodule fixes #561

merged 1 commit into from
Oct 21, 2022

Conversation

cnpetra
Copy link
Collaborator

@cnpetra cnpetra commented Oct 19, 2022

When the release archives are used to build HiOp, git submodule fails since the sources are not under a git repo. Failure can happen when git is simply not available on the machine.

It appears that we need to reintroduce HIOP_EIGEN_DIR to allow user specify location of eigen.

@cnpetra
Copy link
Collaborator Author

cnpetra commented Oct 19, 2022

The current implementation is a draft/discussion started, especially this part: https://github.com/LLNL/hiop/pull/561/files#diff-1e7de1ae2d059d21e1dd75d5812d5a34b0222cef273b7c3a2af62eb747f9d20aR202

Looks like a better way is to factor out code that checks that eigen is present out of the HiopCheckGitModules.cmake and reuse that for when user provides the location of eigen. Maybe a HiOpFindEigen.cmake ? Comments & suggestions will be appreciated.

@cnpetra
Copy link
Collaborator Author

cnpetra commented Oct 19, 2022

reviewers, PR was tested with hiop source from release archive and hiop source under git

@pelesh
Copy link
Collaborator

pelesh commented Oct 19, 2022

@cnpetra, how essential Eigen dependency will be for HiOp in the future? If you anticipate you would use eigen for most of HiOp configurations going forward, perhaps the easiest would be to require user to always clone HiOp recursively (see e.g. RAJA and BLT).

Another option could be to use Spack to get Eigen as we do for other dependencies. Eigen package is in Spack upstream.

I am not necessarily favoring these options, but might be worthwhile considering them at this time.

@cnpetra
Copy link
Collaborator Author

cnpetra commented Oct 19, 2022

@cnpetra, how essential Eigen dependency will be for HiOp in the future? If you anticipate you would use eigen for most of HiOp configurations going forward, perhaps the easiest would be to require user to always clone HiOp recursively (see e.g. RAJA and BLT).

Another option could be to use Spack to get Eigen as we do for other dependencies. Eigen package is in Spack upstream.

I am not necessarily favoring these options, but might be worthwhile considering them at this time.

My preference is to have eigen (and any other dependencies) available without git and spack.

@pelesh
Copy link
Collaborator

pelesh commented Oct 19, 2022

My preference is to have eigen (and any other dependencies) available without git and spack.

If Eigen does not have a special dependency status, then perhaps we should not provide an "extra service" to the user through git submodules. Instead, we could use the same CMake find approach as for other HiOp dependencies, e.g. UMFPACK.

Copy link
Collaborator

@nychiang nychiang left a comment

Choose a reason for hiding this comment

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

lgtm

@cameronrutherford
Copy link
Collaborator

I think we can just merge this to resolve #556.

I don't see any reason why Eigen should be managed as a submodule instead of through Spack like our other dependencies. Perhaps we should plan to transition this to the Spack package with the next release of HiOp?

Copy link
Collaborator

@pelesh pelesh left a comment

Choose a reason for hiding this comment

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

I suggest using same CMake find approach for Eigen as we use for other dependencies. In the meantime, we can merge this to address immediate issues.

@cnpetra
Copy link
Collaborator Author

cnpetra commented Oct 19, 2022

I think we can just merge this to resolve #556.

I don't see any reason why Eigen should be managed as a submodule instead of through Spack like our other dependencies. Perhaps we should plan to transition this to the Spack package with the next release of HiOp?

by managing through spack you mean to have -DHIOP_USE_EIGEN and -DHIOP_EIGEN_DIR ?

@cameronrutherford
Copy link
Collaborator

I think we can just merge this to resolve #556.
I don't see any reason why Eigen should be managed as a submodule instead of through Spack like our other dependencies. Perhaps we should plan to transition this to the Spack package with the next release of HiOp?

by managing through spack you mean to have -DHIOP_USE_EIGEN and -DHIOP_EIGEN_DIR ?

Yes. I am working through whether we can do the same for other submodules in ExaGO. It would be nice to have spack be the only submodule in both projects. cc @pelesh

@cnpetra cnpetra merged commit 3172f62 into develop Oct 21, 2022
@cnpetra cnpetra deleted the gitsubmodule_fix branch June 30, 2023 04:50
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.

4 participants