Add GLOBBING support to target_sources?

Development
1

Currently here are 2 common patterns in our CMake codebase. Since we really like the safe globbing functionality added in 3.13.

What is already possible

# SAFE GLOBBING! :D
file(GLOB_RECURSE sources CONFIGURE_DEPENDS *.cpp *.h)
target_sources(foobar PRIVATE ${sources})

# or

# SAFE GLOBBING! :D
file(GLOB sources CONFIGURE_DEPENDS *.cpp *.h)
target_sources(foobar PRIVATE ${sources})

Proposal

I think it would be great to add this support inside target_sources

# To get recursive behavior specify the keyword RECURSIVE
target_sources(foobar RECURSE PRIVATE *.cpp *.h)

# Default is NON-RECURSIVE
target_sources(foobar PRIVATE *.cpp *.h)

Thoughts?

2

I wouldn’t call that “safe globbing” myself. Personally, I find globbing to always be a poorer alternative to explicit source listings. See this post for some details. I don’t think this is something I’d like to see CMake implement (IMO, file(GLOB CONFIGURE_DEPENDS) should go away too, but all I can do is not recommend it at this point), but I won’t stop anyone from implementing it; just trying to use it in codebases I maintain.

3

There is one material difference to this form of proposal which I have noted elsewhere and which we briefly discussed… making glob expressions opaque (i.e. a special part of the SOURCES property or a $<GLOB:...> genex whose result cannot be inspected by the configure step) would, in theory, allow CMake to resume from the generation step and regenerate only the part of the build system affected by the genex. This would lead to faster regeneration than either the existing explicit file lists or CONFIGURE_DEPENDS-style globbing.

Since the key constraint here is opacity from the rest of the configure step, an interesting alternative could be to add a $<SOURCE_LIST:..> genex which loads sources from a file with a newline-delimited list of sources (or json or whatever, no need to bikeshed). Then when you add a source to the list, it could do a fast regeneration without fully reconfiguring.

4

I agree with @ben.boeckel, this is a bad habit to rely on globbing technique.

@buildSystemPerson Any introduction of new behavior requires specific keywords. Glob expansion cannot be done implicitly on target_sources. So keywords GLOB or GLOB_RECURSE are required:

# To get glob recursive behavior 
target_sources(foobar GLOB_RECURSE PRIVATE *.cpp *.h)

# To get glob behavior
target_sources(foobar GLOB PRIVATE *.cpp *.h)

@alex Currently, specified sources are stored in SOURCES property and this property is available during configuration time. So it is not reasonable to use genex to specify sources because it will break the current behavior (genex are evaluated at generation time).

5

That’s not a reason to not have it. One can already do $<PLATFORM_ID:Linux,linux_impl.cpp> in the source listing with the intended effects.

1 Like
6

@ben.boeckel I have read that post. But I still think the benefits out way the cons.

I’ve used it on personal projects and never had any issues with it, and I’ve convinced my team at work to use it.

Maybe after a while I’ll agree with you and @marc.chevrier. But for now we are going to be using this technique.

7

FWIW, I would push back pretty hard against a proposal to add globbing to target_sources(). I’m in agreement with Ben and Marc, I really wouldn’t recommend projects use a globbing-based strategy. The reasons are well-documented by now and for those who still want to use globbing despite this, the tools are already there with file(GLOB) and file(GLOB_RECURSE). We want the commands to help people follow good practices, and I think the proposal mentioned here and similar suggestions around the topic are not consistent with that.

1 Like
8

Based on all the feedback here I’m definitely not going to spread my GLOB pattern to other code bases.

But I’d like to keep on using it on our team, because I like to learn the hard way. Maybe I’ll make a post that shows everything that went wrong when we started using file(GLOB …).

9

We already have genex in SOURCES—$<TARGET_OBJECTS:...> for example—so I truly do not understand your comment. The fact that INTERFACE_SOURCES exists and can contain genex makes computing the actual list of sources at configure time impossible already. This is the status quo.

Adding $<SOURCES_LIST:...> (not a glob!) would purely enable faster regeneration since if the named file changes, the whole configure step would not need to run.

10

One annoying issue is that CONFIGURE_DEPENDS breaks dry runs (-nv) on Ninja: all you see are the glob checks and recursive invocations. On the other hand not using CONFIGURE_DEPENDS with file(GLOB) requires your build’s users to manually re-run CMake, which is even worse.

I wonder if it would be reasonable to make CONFIGURE_DEPENDS the default in project mode via a policy and add NO_CONFIGURE_DEPENDS to opt-out instead…

12

Minor update. After using globbing for a while on a real production codebase.

I’ve come to the same conclusion as the CMake team. Don’t do it.

It’s nice at first, and tempting for small projects that are constantly changing/adding files. I think that’s where the motivation comes from. When you are starting to write a new project. Because it’s annoying to update the CMake code when you are changing things constantly.

But on a real established project where you have to be precise and file names don’t just change arbitrarily it doesn’t really help. And it just causes issues/confusion.

Also (this opinion is subjective) manually adding the files really gets you thinking about the names of files, directories, and the layout of your project. This one is subjective but I found it to be true with multiple developers.

It was still nice though for maintaining 2 builds systems (we were in the process of moving to CMake from a custom build system). Also we weren’t using CMake for installation, packaging, …, yet. (That’s where globbing files is really awful don’t do it if you are planning on making a library to be consumed by other users).

I’ve come around. Don’t use globbing for source files or header files. And I actually want a way for top level projects to disable it now.

5 Likes