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

flexible dependency detection #93

Closed
loriab opened this issue Jan 25, 2024 · 7 comments · Fixed by #95
Closed

flexible dependency detection #93

loriab opened this issue Jan 25, 2024 · 7 comments · Fixed by #95

Comments

@loriab
Copy link

loriab commented Jan 25, 2024

Background: Most Psi4 builds (by devs, users, and packagers) detect pre-built installations of its dependency packages, rather than building them with FetchContent (hereafter FC), including gau2grid (g2g).

  • From email discussions, it sounds like gauxc includes a pre-generated g2g source and expects to build that through FC. It'd be handy to have the option to detect a prebuilt and to not have two g2g libraries running around.
  • Psi4 has no problems requiring CMake ~3.24 to use FetchContent(... FIND_PACKAGE) if that'll help.
  • I can change any CMake for the g2g repo if that'll help.
@wavefunction91
Copy link
Owner

@loriab See #95, let me know if it fixes the issue for Psi4 integration. The fix assumes the gau2grid::gg target is visible or discoverable, that should cover the bases.

@loriab
Copy link
Author

loriab commented Mar 6, 2024

Sorry for the delay. I think this would work cleanly if gauxc was fetchcontent'ed because then gau2grid::gg would have already been detected or built. Since Psi4 uses superbuild+add_externalproject, I suspect that the above-changed file is run at cmake configure-time, so the gau2grid::gg target _isn't there. Nevertheless, I bet I can mislead it with a dummy g2g target, and all will be fine. The main thing is that the internal g2g build is avoidable, and this fulfills that. Thanks!

@wavefunction91
Copy link
Owner

The gau2grid::gg garget isn't there

Hmm, ok. How do you guys handle your superbuild dependency tree (i.e. how do you ensure that dependencies are seen and propagated throughout the configure)? I'm happy to add something that makes this easier (short of adding psi-specific configure logic)

@loriab
Copy link
Author

loriab commented Mar 14, 2024

Yeah, it looks a little weird in the FetchContent era, but (using libxc as an example) we create a dummy target libxc_external https://github.com/search?q=repo%3Apsi4%2Fpsi4%20libxc_external&type=code that either already exists (detected pre-built) or will exist by the time psi4 itself compiles (need to build from src) and pass that as a dep to the Add_ExternalProject(psi4-core). By the time psi4-core is processing deps w/i the superbuild, Libxc::xc is present and detected, https://github.com/psi4/psi4/blob/297589e976e605c8bca77542b49cfb9f61231322/psi4/CMakeLists.txt#L226-L227 .

So, right, I couldn't think of a tidy way to improve your #95 for us. I figured I'd make a add_library(gau2grid::gg ALIAS gau2grid_external) before the Add_ExternalProject(gauxc) which would then fool your logic into not building g2g. Then since CMake forgets everything btwn the outer cmake pass (where all the Add_EPs are processed) and the internal one (where all the deps are redetected) (the only info passed is through https://github.com/psi4/psi4/blob/master/CMakeLists.txt#L256-L332 and a few Target files), that fake gau2grid::gg is forgotten and redetected to actually mean the target in https://github.com/psi4/psi4/blob/master/psi4/CMakeLists.txt#L146-L147 .

I haven't actually tried this, so please lmk if it needs to be tested b/c getting locked into a release and can't be changed easily or something.

@wavefunction91
Copy link
Owner

wavefunction91 commented Mar 14, 2024

We don't have hard release structure at the moment (though I suppose we should start doing that...), so there's no issues there. I would like to make sure that this works in your build system though, as it would be the main user of any non-FetchContent configure path

@loriab
Copy link
Author

loriab commented Mar 14, 2024

Ok, I think David Poole and I need to look at the Psi4+GauXC buildsys psi4-side together, so we'll add this patch and report back. Will next week work with your timeframe?

@wavefunction91
Copy link
Owner

Yep!

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 a pull request may close this issue.

2 participants