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

CMake compilation as a subdirectory #133

Merged
merged 2 commits into from
Dec 9, 2023
Merged

Conversation

joguenzl
Copy link
Contributor

@joguenzl joguenzl commented Dec 1, 2023

Hello everyone,

when compiling trexio with CMake as a subdirectory of a larger project, e.g. with the following structure:

Project/
  src/
  external/
    trexio/

including trexio in the root CMakeLists.txt as add_subdirectory(external/trexio) has CMAKE_SOURCE_DIR pointing to Project instead of Project/external/trexio which breaks functionality. It is fixed by using PROJECT_SOURCE_DIR instead, which is identical to CMAKE_SOURCE_DIR if trexio is installed on its own.

Furthermore, I made FindTREXIO.cmake check whether TREXIO_DIR is set as a CMake variable before loading it from the environment variable.

@q-posev
Copy link
Member

q-posev commented Dec 1, 2023

Thanks a lot @joguenzl, looks good to me! Just need to confirm one thing before merging.

@wavefunction91 - this is partially related to what we have discussed about making TREXIO compatible with FetchContent functionality. I believe you would need to substitute CMAKE_SOURCE_DIR with PROJECT_SOURCE_DIR the way @joguenzl did in this PR. Is there no conflict with the work you were planning to do for FetchContent?

@q-posev
Copy link
Member

q-posev commented Dec 1, 2023

MacOS CI is broken because I need to bump the min numpy version requirements...

@q-posev q-posev self-requested a review December 1, 2023 11:01
Copy link
Member

@q-posev q-posev left a comment

Choose a reason for hiding this comment

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

I think we can merge as is.

@q-posev q-posev merged commit cea5f1b into TREX-CoE:master Dec 9, 2023
2 of 3 checks passed
@wavefunction91
Copy link

@q-posev ah, sorry. Didn't see the original ping. Yes, this was one of the changes we had to make, exactly in the way we made it. No conflict at all!

@q-posev
Copy link
Member

q-posev commented Dec 10, 2023

@wavefunction91 no problem, thanks for the update!

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.

3 participants