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 support for conditional dependencies via INCLUDE_IF keyword. #394

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

itavero
Copy link

@itavero itavero commented Aug 24, 2022

Still a work in progress. See #391 for more details.

To do (amongst other things probably):

  • Write tests
  • Update documentation

if(DEFINED CPM_ARGS_INCLUDE_IF)

# Filter out INCLUDE_IF from arguments
cpm_remove_one_value_arg(INCLUDE_IF ARGN ${ARGN})
Copy link
Author

Choose a reason for hiding this comment

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

This function currently assumes that the user has always supplied a value for INCLUDE_IF.
That might be okay, but for CMake 3.15 and newer, we could also check if INCLUDE_IF is not in CPM_ARGS_KEYWORDS_MISSING_VALUES and otherwise inform this function about the fact that there is no value to be removed (see cmake_parse_arguments docs.

@iboB
Copy link
Member

iboB commented Sep 8, 2022

I'm against this. I'm pasting my comment from the linked discussion:

The proposed solutions would only work if all packages are listed in a central CMakeLists, and if this was the case, there would not be a point in having a package-lock. This central CMakeLists can play the same role.

But packages are listed in multiple CMakeLists files. My root can have code like:

if(BUILD_SERVER)
    add_subdirectory(server)
endif()
if(BUILD_CLIENT)
    add_subdirectory(client)
endif()

In such case the dependencies of the client won't even be mentioned, if only BUILD_SERVER is true and vice versa. And yes, as @itavero mentioned, different build flavors may require different versions of the same package, leading to an adverse package lock.

Thinking about all edge cases and different scenarios I'm even inclined to propose ditching package-lock altogether. Has anyone made a good use of it?

@iboB
Copy link
Member

iboB commented Sep 8, 2022

I think that package-lock should be at least rethought before proceeding with such changes

@TheLartians
Copy link
Member

Yeah I definitely hadn't given it enough thought until now. Also sharing my comment from the discussion for completeness.

That's a really good point, package-lock definitely isn't compatible with conditional includes. Also to properly update the lock for nested dependencies we would need to somehow query any disabled packages as well making this feature much more difficult to implement than I initially anticipated.

I'm still in favour of keeping the feature, as having a package lock is crucial if we want to support automatic package updates (e.g. #318, #389), but we should make it clear that it doesn't work with conditional includes. As for INCLUDE_IF, it is still possible but would require running a separate temporary CMake process to resolve any nested dependencies when updating the package lock.

Going forward I do think the feature needs a hefty update and be used together with dynamic version tags as suggested in #318.

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.

None yet

3 participants