FetchContent_Declare populate fails with option EXCLUDE_FROM_ALL

ExternalProject_Add() also supports this option, and I was under the impression that FetchContent_Declare() supported all the same options.

It only supports a subset of the options. The FetchContent_Declare() documentation states the following (emphasis added):

The <contentOptions> can be any of the download or update/patch options that the ExternalProject_Add() command understands. The configure, build, install and test steps are explicitly disabled and therefore options related to them will be ignored.

The EXCLUDE_FROM_ALL keyword of ExternalProject_Add() is a configure option and is therefore not supported. As a workaround, you could use the EXCLUDE_FROM_ALL directory property to essentially achieve the same thing. You can either set it on the current directory before calling FetchContent_MakeAvailable() or set it after the directory has been pulled in. For example:

# Case 1: Set before (applies to current scope and below)
set_directory_properties(PROPERTIES EXCLUDE_FROM_ALL YES)

FetchContent_Declare(lib
    GIT_REPOSITORY      https://github.com/username/lib.git
    GIT_SHALLOW         TRUE
    GIT_TAG             origin/optional-positional
    EXCLUDE_FROM_ALL    TRUE
)
FetchContent_MakeAvailable(lib)

# Case 2: Set after (applies only to dependency's scope and below)
if(IS_DIRECTORY "${lib_SOURCE_DIR}")
    set_property(DIRECTORY ${lib_SOURCE_DIR} PROPERTY EXCLUDE_FROM_ALL YES)
endif()

The if(IS_DIRECTORY) test for case 2 is to handle the scenarios where the dependency isn't pulled in via add_subdirectory() as a result of the call to FetchContent_MakeAvailable(). For your particular case, you expect it to, but I'm showing it here for completeness because there are scenarios where that won't be true for other people and there may be future changes which could add more scenarios where that's the case (I have some specific ones in mind for future work related to find_package() integration).

Sorry to say I can't get either to work. I tried case #1 and #2, and still the only thing that works is my second example. I can quickly test since I am fetching a simple header only port of argparse to my main project. Using your options, when I run cmake --build . --target install i get the output

-- Install configuration: "Release"
-- Installing: C:/Program Files (x86)/WC-Programmer/bin/SiUSBXp.dll
-- Installing: C:/Program Files (x86)/WC-Programmer/lib/cmake/argparse/argparseConfig.cmake
-- Installing: C:/Program Files (x86)/WC-Programmer/include/argparse/argparse.hpp
-- Installing: C:/Program Files (x86)/WC-Programmer/bin/wc-programmer.exe

with my second example I get the output

-- Install configuration: "Release"
-- Up-to-date: C:/Program Files (x86)/WC-Programmer/bin/SiUSBXp.dll
-- Up-to-date: C:/Program Files (x86)/WC-Programmer/bin/wc-programmer.exe

Also, please note that in your example including the line EXCLUDE_FROM_ALL TRUE in the FetchContent_Declare call makes it fetch nothing. This causes the configure step to error out since it can't find the source files. It would be nice if it either ignored the option or give a more descriptive error.

Yes, the EXCLUDE_FROM_ALL TRUE left in the FetchContent_Declare() call of my example was a cut-n-paste error on my part.

I couldn't find it earlier, but I managed to track down !3930 (closed) where this was discussed previously.

Regarding the directory property not working, I decided to investigate and unfortunately it looks like there is a previously undiscovered bug with a murky and complex history. I just reported #20168 (closed) which may explain what you've seen. I won't go into more detail here, there are lots of fixes, reverts and back-ports involved and there's clearly still more to fix. For now, I think you can test with CMake 3.14.6 or earlier, or 3.15.0 through to 3.15.3 and still have things work for your particular case.

thank you for investigating. Due to #20177 I won't be using this pattern for now anyhow, and instead the practice you mention in #18628, using CMAKE_INSTALL_DEFAULT_COMPONENT_NAME around all fetch/include_directory. This makes cmake --install . more or less useless but at least makes CPack work nicely.

Hello. I would also be very interested in using FetchContent without fetched libraries polluting the ALL target and installed. Can FetchContent_MakeAvailable not simply have an EXCLUDE_FROM_ALL option that would be passed down to add_subdirectory ?

Can FetchContent_MakeAvailable not simply have an EXCLUDE_FROM_ALL option that would be passed down to add_subdirectory ?

No, FetchContent_MakeAvailable() cannot have any options like that. All the options need to come from FetchContent_Declare(). There might be more than one place that needs a particular dependency, so you can't in general rely on any particular FetchContent_MakeAvailable() call being the one that actually pulls in a given dependency (it might have already been pulled in by another call earlier). I have some upcoming work to do on FetchContent / find_package() integration which will hopefully unblock adding the necessary support to FetchContent_Declare() for this, but I don't want to add it prematurely in case it interferes with that work.

assigned to @craig.scott

mentioned in issue #21687 (closed)

Hi! Any updates on this topic? It would be really neat to have EXCLUDE_FROM_ALL available in FetchContent_Declare().

mentioned in issue #23710 (closed)

Hi @craig.scott! Is there any appetite for having this issue addressed externally? I have some changes working locally I was considering submitting a patch for and stumbled across this issue.

Now that the find_package() integration is done, I think we can revisit this one. If you'd like to work on it, I suggest looking at how the SYSTEM keyword was added to FetchContent in !7399 (merged). A new EXCLUDE_FROM_ALL keyword could probably be implemented in a similar fashion. There are a few things we would need to carefully consider though:

ExternalProject_Add() already has an EXCLUDE_FROM_ALL option, and while the effect of the proposed EXCLUDE_FROM_ALL option to FetchContent_Declare() would be similar, it would need to be clearly documented as a special case. FetchContent doesn't support any of the configure, build, install or test options from ExternalProject_Add(), but this would be a case where it would appear that we do. We have a similar situation with SOURCE_SUBDIR, so there is precedent, but we would need to be very careful.
Another complication is that the SYSTEM argument to FetchContent_Declare() doesn't take an argument. That keyword is either present or not. This was done to make it similar to the way the SYSTEM keyword is used with add_subdirectory(). Unfortunately, that now creates a problem for EXCLUDE_FROM_ALL. We could follow the same reasoning and implement it as a present-or-not case as well, but then it would be different the existing EXCLUDE_FROM_ALL option that ExternalProject uses. Even though the two options are different, I think this would create confusion. I think either way, we are now stuck with either choice creating an inconsistency. Clear documentation will help, but inevitably people won't read that and will get things wrong, potentially in subtle ways that may go unnoticed but give unintended behavior.
Adding any new keywords to FetchContent_Declare() has strong implications for dependency providers. They have to parse the arguments themselves, which means they would all have to be updated to handle any new keywords. In the long term, a solution to that is probably to provide some sort of argument parsing command such that they can just ignore arguments they don't implement (potentially dangerous, but often ok and only suboptimal, as would likely be the case here). I haven't thought through that functionality enough yet though. It is made harder by the need to support the GIT_SUBMODULES keyword, which has different behavior between no arguments following it and an explicit single empty argument following it (there are implications for argument forwarding).

If you do want to work on this, I'd prefer targeting CMake 3.28 rather than 3.27, if you'd be ok with that. The release cycle for 3.27 will probably start in a few weeks, and I'd be much more comfortable with a longer period of a new EXCLUDE_FROM_ALL feature being merged to master and getting some extended testing before being included in a release. I'm also hoping to focus on bug fixes for ExternalProject and FetchContent in the time remaining before the 3.27 release cycle starts.

Excellent - thanks for the detailed response! I had some of the same thoughts as well, especially the question of supporting EXCLUDE_FROM_ALL as a bare keyword versus the typical EXCLUDE_FROM_ALL TRUE used with ExternalProject.

What would you think of going with the bare keyword as an argument to FetchContent_Declare()? It might be a bit easier to handle in the current implementation of FetchContent.cmake since there are already special provisions for keywords with arguments being forwarded to ExternalProject_Add(). I absolutely agree on adding copious amounts of documentation - my secondary hope is that if someone attempts to add an argument to EXCLUDE_FROM_ALL and receives a failure, that will force them to look up usage.

I hadn't considered the effects on dependency providers (this is still a very new feature to me), but I'm happy to spend some time digging around and seeing if a pattern develops.

As far as targeting 3.28, that sounds reasonable to me. It sounds like this will be a sizable change - what is the best process for moving forward? Is it enough to open an MR and tag this issue with you listed as a reviewer?

On an unrelated note, one of the workarounds I've used for this issue is the creation of a helper module with a macro named FetchContentHelper_FindPackage(). It's effectively a stripped-down version of FetchContent_MakeAvailable() that allows me to define find modules that modify populated content without affecting the list file. It's a little different than the usual semantics of using find modules with FetchContent.cmake, but in practice it has been working very well and (to me at least) feels like a natural extension to OVERRIDE_FIND_PACKAGE. As a follow-on to this work (in a separate MR), do you think there would be any interest in adding this macro with additional documentation to describe this use case?

What would you think of going with the bare keyword as an argument to FetchContent_Declare()?

Let's go with that. Follow the same pattern as used for SYSTEM. I'll keep an eye out for any problems with interacting with the existing EXCLUDE_FROM_ALL keyword handling that ExternalProject already has.

I absolutely agree on adding copious amounts of documentation

That's not the goal. The aim is for very clear and concise docs. The bigger problem is that people won't read the docs at all and make assumptions about how things work based on their existing knowledge.

...my secondary hope is that if someone attempts to add an argument to EXCLUDE_FROM_ALL and receives a failure, that will force them to look up usage.

Part of the problem here is that because of how ExternalProject and FetchContent parse their arguments, it has quite poor error-checking. If you get things wrong, it is very possible to get no error, but very strange or unexpected behavior (and if you're unlucky, subtly wrong behavior you may not immediately notice).

...what is the best process for moving forward? Is it enough to open an MR and tag this issue with you listed as a reviewer?

Open a merge request and add Fixes: #20167 to the footer of the commit message. No need to assign reviewers or approvers, we manage those things ourselves. You should definitely take a look at the contributors guide before getting started. The CMake Review Process page should be particularly helpful in explaining the process.

...one of the workarounds I've used for this issue is the creation of a helper module with a macro named FetchContentHelper_FindPackage()...do you think there would be any interest in adding this macro with additional documentation to describe this use case?

Please open a new issue to discuss that rather than here so we can keep things focused.

Let's go with that. Follow the same pattern as used for SYSTEM. I'll keep an eye out for any problems with interacting with the existing EXCLUDE_FROM_ALL keyword handling that ExternalProject already has.

Sounds good. I have some candidate changes ready to go, but I ran into a snag. One of the surprising things I found is that EXCLUDE_FROM_ALL isn't applied as a directory property, meaning that it is not applied transitively like SYSTEM is. In my branch, I've made this change, however this is different behavior than before and I suspect it will break something somewhere. Without this change, the gains from applying EXCLUDE_FROM_ALL are minimized when fetching projects that they themselves fetch dependencies.

It seems like the right thing to do here is introduce a new policy, but before doing that work I wanted to ask what your opinion was. Testing large packages like Boost or Hello ImGui yield good results with this new behavior enabled (~50% reduction of built translation units).

That's not the goal. The aim is for very clear and concise docs.

Will do.

Open a merge request and add Fixes: #20167 to the footer of the commit message.

Thanks! As soon as we can nail down how to handle a potential policy change, I can put an MR up. I've added additional tests and so far everything is looking clean on my machine. I'm curious what CI/CD will reveal.

The first change should just be restricted to passing through EXCLUDE_FROM_ALL to the add_subdirectory() call made by FetchContent_MakeAvailable() (except where re-routed by find_package() integration or a dependency provider). That's the first fundamental change we need. For the second part about whether add_subdirectory(... EXCLUDE_FROM_ALL) is behaving how you expect, I suggest you base your experiments on actually doing a "make all" (or its equivalent) and seeing what happens. Looking at the directory properties alone might lead you to think you'll get one outcome, but the actual behavior may be different. The directory property not propagating down to deeper subdirectories is likely to mislead you in this case. You may also want to take a look at #20168 (closed) and #20168 (comment 675546) in particular. I believe that discussion already raised the same thing as you have here and it was concluded the behavior is correct (but by all means, please do further experiments to confirm if you think that isn't the case).

For the removal of doubt, I don't think we need a policy here for the part that is just adding support for EXCLUDE_FROM_ALL to FetchContent.

I apologize for taking so long to respond - work has been a bit hectic lately. This all sounds good, I should be able to wrap up documentation this week and submit an MR.

@sstallion Just checking if you're still intending to work on this? It would be good to get the MR up in the next couple of weeks if we're going to aim for inclusion in CMake 3.28. I have some other changes in mind that will be blocked by this, so for planning purposes, I'd like to get an idea where you're at with this work. If we're not aiming for 3.28, that's fine, I'm mostly just looking ahead for how I plan my own contributions in the next month or two.

Hey @craig.scott - I apologize I've been bogged down at work. I have the necessary changes made (they aren't large), I just need to add documentation and additional tests. Testing these changes is proving difficult - I can't for the life of me come up with a sane method of determining whether a subdirectory has been added or not from a top-level list file. Any advice would be very welcome!

You could add a target in the dependency's CMakeLists.txt file, and the source for that target could contain a line like:

#error This file should not get compiled

Then, the test would build the default all target of the top level project. If EXCLUDE_FROM_ALL is working correctly, then the dependency target won't get built and its source won't be compiled. If EXCLUDE_FROM_ALL is not working, then the source will get compiled, but the build will error out due to the line above. If you search for #error under the Tests/RunCMake subdirectory, you should find plenty of examples doing something similar.

Ah! That makes much more sense than the mess I have - Thanks! I'll get this tied up this week and an MR opened.

@craig.scott A quick update - changes are finished and a minimal set of tests are ready to go. I should be able to finish up documentation in the next day or two and I should be ready to open an MR. Thanks for your patience!

mentioned in merge request !8822 (merged)

closed with merge request !8822 (merged)

mentioned in commit 5f77807f

FetchContent_Declare populate fails with option EXCLUDE_FROM_ALL

Child items ...

Activity