-
Notifications
You must be signed in to change notification settings - Fork 190
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
Support CPM_<dependency name>_SOURCE
local package override from ENV
#406
base: master
Are you sure you want to change the base?
Support CPM_<dependency name>_SOURCE
local package override from ENV
#406
Conversation
CPM_<dependency name>_SOURCE
local package override from ENV
There was a problem hiding this 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! I can see the use in having additional configurability for CPM and packages.
However, I'm unsure about the extra support for environmental variables, as I want to avoid having competing methods for the same functionality when not needed. Also I'm a bit sceptical of adding too much flexibility using the environment, as I can see issues arising when users forget to clear their env variables and are getting unexpected CPM / package versions.
cmake/get_cpm.cmake
Outdated
set(CPM_DOWNLOAD_LOCATION "$ENV{CPM_SOURCE_CACHE}/cpm/CPM_${CPM_DOWNLOAD_VERSION}.cmake") | ||
else() | ||
set(CPM_DOWNLOAD_LOCATION "${CMAKE_BINARY_DIR}/cmake/CPM_${CPM_DOWNLOAD_VERSION}.cmake") | ||
if ( NOT CPM_DOWNLOAD_LOCATION) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An unwanted side effect of keeping the value of CPM_DOWNLOAD_LOCATION
is that users would no longer receive a warning when a upstream package uses a newer CPM version than the outer project. This is currently handled inside the newer CPM.cmake script that would be included.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I can just about see the issue you refer to here but maybe not, patient with me for a moment....
I may not be right, but for my current use-case I am using CPM_DOWNLOAD_LOCATION
to override a projects CPM version at the top of the build. However until all the packages on which it depends are also updated to contain this check the CPM version will not be set across the entire build. However when/if this change is integrated into all packages then they would all end up using the same CPM script file.
In my view this being able to set the entire build to use a single CPM script feels preferential for developing than to supporting and testing having mixing of CPM versions across packages? It also allows testing if a dependency would have issues upgrading to a new CPM for instance with ease etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I may not be right, but for my current use-case I am using CPM_DOWNLOAD_LOCATION to override a projects CPM version at the top of the build. However until all the packages on which it depends are also updated to contain this check the CPM version will not be set across the entire build. However when/if this change is integrated into all packages then they would all end up using the same CPM script file.
That is true.
In my view this being able to set the entire build to use a single CPM script feels preferential for developing than to supporting and testing having mixing of CPM versions across packages? It also allows testing if a dependency would have issues upgrading to a new CPM for instance with ease etc.
In the current version, we also don't mix CPM functions. Currently, if a more recent CPM version than used downstream is included, a warning is emitted notifying the user that a newer version is available.
While not essential, we should be aware that this functionality would not persist with the suggested change.
I see your view. But in my current use-case it is the opposite. My use case is essentially: Without the ENV option this becomes very error-prone as a clean checkout of one projects dependency ends up pulling in the last version in GIT instead of the latest expected local version. As documented the 💡 Chucking ideas on the table here, an alternative idea to using |
I've had a less intrusive idea to circumvent the need for this PR that could circumvent using the ENV variables but does require the consuming project to include a code change: This is not as EDIT: This approach appears to only be good when you own or can make changes to the package scripts. Unless it could be viable to add some extension point from CPM itself to do the 'include'. Then support having something a bit like GIT with its config scope repo|user|global but for packages so you could override local-directory at differing config locations! @TheLartians Let me know how you feel and I can split/remove the |
I see the issue, my assumption was that usually you would want to test an updated dependency one project at the time without impacting others. Overwriting a package system-wide may create unexpected side-effect as e.g. if the user is unaware of other potential uses of the dependency in other projects. Perhaps an alternative to update a dependency system-wide would be to the dependency in the
If you have access to the outer project, why not directly use a custom
I think splitting the PR could definitely make the discussion more focussed, but for now I'm ok with trying to fully understand the use-cases here as well. |
This is a very good question. I should elaborate further in that I have access to all the projects so could roll my own solution.... which is why this PR came about. So the situation is we have a dependency tree that have multiple layers and we need to test integration at each layer of-the-onion so to speak. The outer-most multiple projects (applications, firmware etc) depend on shared component projects. These components in turn depend on common framework projects, components, or other libraries that are common across the system of applications and components. I think the use-cases can be worded in two core perspectives:
Sounds good. I am wondering if the title of the PR could be better summarized: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the changes, I agree that using CPM_PATH
is more consistent with other naming conventions used so far. I'm happy with changes regarding the CPM and package overrides through CMake variables, but am still very uncertain about the behavioural changes based on environmental variables.
When developing 'Component X' I wish to build & test the changes for API stability by integrating them into multiple application projects that depend on the component
- Global override lets us test a 'suite' of applications without modifying their code.
- Allows detecting API breaking issues earl
- Component development confirm compatibility early or inform maintainer for Application X.
- aka. As maintainer of
Project-Z
I could build every Github project that uses it against my latest source-code to make sure my optimization works and the API doesn't break compatibility.When developing multiple Applications A,B, & C I wish to develop, build, & test against the latest Component-X so I can integrate new features
- Applications can share a common dependency version but are part of a separate build process
- Can simplify Component development by integration testing new features across multiple applications
AFAICT both cases are solved without touching the source code by defining the CPM_<X>_SOURCE
either explicitly in the build cmake <path to application> -D CPM_<X>_SOURCE=<path to X>
or implicitly as suggested by this PR using env variables CPM_<X>_SOURCE=<path to X> cmake <path to application>
. Same would apply to CPM.cmake itself using the CPM_PATH
variable after this PR. I'm guessing that the advantage of the environmental approach is that it would make it easy to override packages in complicated external toolchains that invoke CMake implicitly?
@CraigHutchinson thanks for the update! As said before I'm still not really happy with allowing fine grained package or CPM overrides using environmental variables due to unexpected behaviour when variables are set and forgotten. I'm still not sure I understand what the actual pain point is that env variables solve to see if it balances out this risk. Did you see my comment regarding the use-cases above?
Atm I still prefer only allowing to explicitly pass the overrides. |
Feels like this may be related to #336 which adds a per-package |
That's true, IMO that is something we should be re-review as well although it only allows switching between two sources and not arbitrary code injections. But it's also possible I'm being too paranoid here, as it seems to be a desired feature and perhaps the usability / security risks aren't as great as I think. @iboB perhaps you could weigh in here with a third opinion? |
I would like to add to the list of use cases above. In general U, V and W are maintained and released separately. So they might independently bump dependencies towards A. So I can always mirror and fork repositories and enforce a specific version manually. But for setting the specific version I only need to change the CPMAddPackage call within U, V and W. So fork + change seems overly excessive. A notorious example for that could be Boost. It provides so many independent libraries and APIs stay compatible across many releases, meanwhile ABIs may easily change.. So I am wondering if this PR comes close to this use case, or am I missing an existing feature in CPM? |
@APokorny to override the version of |
Yes it does - and now I also get the advantage of specifying a source location with CPM_A_SOURCE. So I can easily configure builids that use library dependencies from local checkouts. This is a great feature! I did not realize that this is possible! |
d4dbec0
to
68a0f74
Compare
One use case for env variables might be CI Builds, where this feature could be useful. As an example the Personally, I don't see a use case for CPM_A_SOURCE in my projects at the moment, but I think in general environment variables can be very helpful. |
This PR has been open a while. Can we try and get a Decisive yay or nay on this soon please. I am using this internally and live by it so I can try and rework the concept if this one isn't going to float 👍 |
I still feel a bit uneasy on this one, as it adds a lot of implicit configuration endpoints that can be hard to monitor and debug. How would you feel about enabling the feature only when a CMake variable like This could be explicitly passed to CMake to enable all environmental control ( |
I don't think using the environment variables would cause much confusion. Typical usages include writing some The mentioned switch option should be good. Better if it is enabled on default in my opinion, but |
CPM_<dependency name>_SOURCE
system wide as EnvironmentCPM_PATH
i.e. Can be set to location of local CPM cloneTODO:
CPM_<dependency name>_SOURCE
for external source DONE: USeCPM_PATH
as to not conflict with dependency variables but make intent clear