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.

Sign up for GitHub

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

Jump to bottom

Respect FETCHCONTENT_BASE_DIR if set by a user #244

Merged
merged 2 commits into from
Apr 16, 2021
Merged

Respect FETCHCONTENT_BASE_DIR if set by a user #244

merged 2 commits into from
Apr 16, 2021

Conversation

trxcllnt
Copy link
Contributor

No description provided.

TheLartians
TheLartians reviewed Apr 15, 2021
View reviewed changes
Copy link
Member

@TheLartians TheLartians left a comment

Choose a reason for hiding this comment

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

Hey, thanks for the PR! It's a good idea to allow users to define custom FetchContent base directories. Imo it might be a good idea to merge the two branches to avoid repetition.

cmake/CPM.cmake Outdated Show resolved Hide resolved
@trxcllnt
Copy link
Contributor Author

It's a good idea to allow users to define custom FetchContent base directories

This PR solves the case where if CPM_SOURCE_CACHE is unpopulated, dependencies are built under FETCHCONTENT_BASE_DIR, but if the cache is already populated they're built in ${CMAKE_BINARY_DIR}/_deps .

TheLartians
TheLartians approved these changes Apr 16, 2021
View reviewed changes
Copy link
Member

@TheLartians TheLartians left a comment

Choose a reason for hiding this comment

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

Looks great, thanks for the changes!

@TheLartians
Copy link
Member

This PR solves the case where if CPM_SOURCE_CACHE is unpopulated, dependencies are built under FETCHCONTENT_BASE_DIR, but if the cache is already populated they're built in ${CMAKE_BINARY_DIR}/_deps .

Shouldn't FetchContent itself respect FETCHCONTENT_BASE_DIR in that case?

@trxcllnt
Copy link
Contributor Author

It does, but in the case the cache is already populated CPM takes the branch with this comment:

avoid FetchContent modules to improve performance

🙃

@TheLartians
Copy link
Member

Ah yes, I think misunderstood your comment. 😅 All good then, thanks for clarifying!

@TheLartians TheLartians merged commit 7644c3a into cpm-cmake:master Apr 16, 2021
@TheLartians
Copy link
Member

Fix is released in v0.32.1 🎉
Thanks for the PR!

This was referenced Apr 16, 2021
Merged
Merged
Merged
Merged
rapids-bot bot pushed a commit to rapidsai/rmm that referenced this pull request Apr 16, 2021
@trxcllnt
Update to CPM with fix for FETCHCONTENT_BASE_DIR (#754)
b899d35 
Update CPM with a [fix for `FETCHCONTENT_BASE_DIR`](cpm-cmake/CPM.cmake#244).

Authors:
  - Paul Taylor (https://github.com/trxcllnt)

Approvers:
  - Keith Kraus (https://github.com/kkraus14)

URL: #754
rapids-bot bot pushed a commit to rapidsai/cudf that referenced this pull request Apr 17, 2021
@trxcllnt
Update to CPM with fix for FETCHCONTENT_BASE_DIR (#7982)
ed68e96 
Update CPM with a [fix for `FETCHCONTENT_BASE_DIR`](cpm-cmake/CPM.cmake#244).

Authors:
  - Paul Taylor (https://github.com/trxcllnt)

Approvers:
  - Keith Kraus (https://github.com/kkraus14)

URL: #7982
rapids-bot bot pushed a commit to rapidsai/cuspatial that referenced this pull request Apr 19, 2021
@trxcllnt
Update to CPM with fix for FETCHCONTENT_BASE_DIR (#386)
728e0fd 
Update CPM with a [fix for `FETCHCONTENT_BASE_DIR`](cpm-cmake/CPM.cmake#244).

Authors:
  - Paul Taylor (https://github.com/trxcllnt)

Approvers:
  - Keith Kraus (https://github.com/kkraus14)

URL: #386
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TheLartians TheLartians TheLartians approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@trxcllnt @TheLartians