Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Allow users to override the default path for CPM_SOURCE_CACHE #669

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

Open
blozano-tt wants to merge 3 commits into cpm-cmake:master
base: master
Choose a base branch
Loading
from blozano-tt:blozano-allow-override

Conversation

@blozano-tt
Copy link

@blozano-tt blozano-tt commented Jun 19, 2025

Currently, only the ENV CPM_SOURCE_CACHE can be used for steering CPM download caching.

However, I am not a fan of environment variable engineering, (We have too much of it already in our code base).

With this change, a user of CPM can first set CPM_SOURCE_CACHE_DEFAULT, and have that be respected by CPM.cmake.

Copy link
Author

@TheLartians does this PR sound okay? Just want to optionally steer this behavior without an environment variable. PS loving CPM over at @tenstorrent

Copy link
Member

iboB commented Jun 24, 2025
edited
Loading

@blozano-tt you can configure with -DCMP_SOURCE_CACHE="some/path". This however contrary to the PR takes precedence over an env var. If it's defined, the env var is ignored.

As a side note here's a piece of code that I use often:

# before including CPM.cmake or get_cpm.cmake
if(NOT CPM_SOURCE_CACHE AND NOT DEFINED ENV{CPM_SOURCE_CACHE})
 set(CPM_SOURCE_CACHE "${CMAKE_SOURCE_DIR}/.cpm" CACHE PATH "CPM source cache")
 message(STATUS "Setting cpm cache dir to: ${CPM_SOURCE_CACHE}")
endif()

This allows me to cache /repo-root/.cpm on a CI, and reduces the download count for multiple config on dev machines which don't have a global setting.

Given this, I would be against merging this PR

Copy link
Author

@blozano-tt you can configure with -DCMP_SOURCE_CACHE="some/path". This however contrary to the PR takes precedence over an env var. If it's defined, the env var is ignored.

As a side note here's a piece of code that I use often:

# before including CPM.cmake or get_cpm.cmake
if(NOT CPM_SOURCE_CACHE AND NOT DEFINED ENV{CPM_SOURCE_CACHE})
 set(CPM_SOURCE_CACHE "${CMAKE_SOURCE_DIR}/.cpm" CACHE PATH "CPM source cache")
 message(STATUS "Setting cpm cache dir to: ${CPM_SOURCE_CACHE}")
endif()

This allows me to cache /repo-root/.cpm on a CI, and reduces the download count for multiple config on dev machines which don't have a global setting.

Given this, I would be against merging this PR

Hi @iboB,

Thanks for sharing your thoughts.

Yes that is possible, and we are doing that today in my project. We also have a similar snippet to what you shared. https://github.com/tenstorrent/tt-metal/blob/c50030e1323c5a4b71627e409146e46b799673a6/cmake/CPM.cmake#L8-L13

Someone complained that our project would not respect the ENV, so if someone reads the CPM docs, they change the ENV in a pipeline it would have no effect... this increaased the complexity of our snippet, and lead me to question ... why can't the default CPM.cmake from upstream absorb this functionality.

With my proposal, CPM users can control the location with a single switch, and no one would need a custom snippet like you shared. Right?

Copy link
Member

iboB commented Jun 25, 2025

Ah. I think I get it now. The goal of this PR is not to make CPM_SOURCE_CACHE_DEFAULT something that one would configure with, but to reduce snippets like yours and mine from several lines to just:

set(CPM_SOURCE_CACHE_DEFAULT "${CMAKE_SOURCE_DIR}/.cpm")

I'm not necessarily against that. However this does not affect just CPM.cmake itself, but also get_cpm.cmake. get_cpm respects the source cache and our snippets working on CPM_SOURCE_CACHE would affect it properly. Such behavior needs to be reflected there as well

TheLartians reacted with thumbs up emoji blozano-tt reacted with heart emoji

Copy link
Author

but to reduce snippets like yours and mine from several lines to just:

set(CPM_SOURCE_CACHE_DEFAULT "${CMAKE_SOURCE_DIR}/.cpm")

Yes, exactly

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!

In general, I'm not a fan of having multiple ways to achieve the same result, as it adds potential sources of confusion. Therefore, we need to be careful when considering extra options like this.

That said, with the current workarounds presented, I see an issue where an upstream dependency using the override could change the default cache location or suddenly enable the cache when reconfiguring a project. I think the suggested change would add a canonical way of setting the default cache location that doesn't have as many potential issues. So IMO it's a good idea to add it.

To get this merged I would request two additions:

  • As @iboB mentioned, could you also update get_cpm.cmake to check for CPM_SOURCE_CACHE_DEFAULT before falling back to no cache?
  • Could you document this option in the readme, so users can see how to set the default cache location? We might want to mention to not use locations outside of the source or build directory to avoid different projects creating different global caches on a system.

Thanks!

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.

Sorry, meant to request changes!

blozano-tt reacted with heart emoji
Copy link
Author

Hey, thanks for the PR!

In general, I'm not a fan of having multiple ways to achieve the same result, as it adds potential sources of confusion. Therefore, we need to be careful when considering extra options like this.

That said, with the current workarounds presented, I see an issue where an upstream dependency using the override could change the default cache location or suddenly enable the cache when reconfiguring a project. I think the suggested change would add a canonical way of setting the default cache location that doesn't have as many potential issues. So IMO it's a good idea to add it.

To get this merged I would request two additions:

  • As @iboB mentioned, could you also update get_cpm.cmake to check for CPM_SOURCE_CACHE_DEFAULT before falling back to no cache?
  • Could you document this option in the readme, so users can see how to set the default cache location? We might want to mention to not use locations outside of the source or build directory to avoid different projects creating different global caches on a system.

Thanks!

Will make the requested changes in a couple weeks after summer vacay!

TheLartians reacted with heart emoji

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@TheLartians TheLartians TheLartians requested changes

Requested changes must be addressed to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /