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

Add per package CPM_DOWNLOAD controls #336

Merged

Conversation

robertmaynard
Copy link
Contributor

Introduces support for CPM_<PACKAGE>_DOWNLOAD variable ( and env ) which allows finer grained control.

A common workflow pattern we have run into is wanting to set CPM_DOWNLOAD_ALL to on for a middle package in a CPM dependency chain.

Root
  -> CPM project A
       -> CPM project B 
       -> CPM project C

This would allow us to state that project A needs to be downloaded but project B or C can be found on disk.

Introduces support for `CPM_<PACKAGE>_DOWNLOAD` variable ( and env )
which allows finer grained control.
README.md Outdated Show resolved Hide resolved
Copy link
Member

@iboB iboB left a comment

Choose a reason for hiding this comment

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

Thanks for the PR

README.md Outdated Show resolved Hide resolved
cmake/CPM.cmake Outdated Show resolved Hide resolved
cmake/CPM.cmake Outdated Show resolved Hide resolved
cmake/CPM.cmake Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@OlivierLDff
Copy link
Contributor

Interesting this like the variant I thought here #280 (but still didn't take time to implement)

@robertmaynard robertmaynard force-pushed the support_per_project_download_flags branch from 43411b4 to b10c7b8 Compare January 27, 2022 14:06
@robertmaynard robertmaynard changed the title Add per project CPM_DOWNLOAD controls Add per package CPM_DOWNLOAD controls Jan 27, 2022
@iboB iboB requested a review from TheLartians January 27, 2022 14:54
Copy link
Member

@iboB iboB left a comment

Choose a reason for hiding this comment

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

I can't think of how this can break anything. Looks good to me.

@TheLartians what do you think?

Also, this makes is a good use case for an integration test, I'll try do add one for this over the weekend

@TheLartians
Copy link
Member

Agreed, this looks good! Definitely worth it to add an integration test, so we don't accidentally break it in the future.

@TheLartians
Copy link
Member

@iboB are you still planning to write some integration tests for this feature?

@iboB
Copy link
Member

iboB commented Feb 8, 2022

Yes. At least a very bare-bones one. I'm a bit short on time, though. I think if we're satisfied with this, we can merge and I can add tests later

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.

Alright, thanks @robertmaynard for the feature!

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.

4 participants