OOS builds and relative exclude paths

[kjerstadius]

I'm using the current tip of master (c018e2b) and what I'm trying to do is to define some excludes using a gcovr.cfg file. When combining this with an OOS build (in my case Meson, but the same principle would apply for CMake too) I find that I'm unable to define exclude path(s) in a satisfactory manner. If I'm not mistaken, the problem I see arises because of the fact that relative filters are always generated relative to the current working directory, which in an OOS build will never be the same directory as the sources are in (except for generated sources, which may complicate things further). This means that in order to exclude a source file from the report, you either need to know the absolute path to the file, or the relative path to the file from the build directory. Since neither of these are known such that they can be defined once, the prospect of having a gcovr.cfg file with excludes defined sort of falls apart.

The way I see it there are a couple of different options that could help solving this issue (in no particular order):

• Allow relative filters to be built relative to the source root by passing an additional (optional) argument to build_filter().
• Allow gcovr.cfg files to be located in the build root.
• Add some kind of expression or syntax to tell gcovr that an exclude is or isn't relative to the source root.

I already tried the first approach by making some minimal changes on my fork (see here, I've not yet made a PR of it since I wanted to discuss first). It works for my simple test and passed all CI tests, but I'm not sure whether there may be unknown (to me) side effects, or if other filters should be given the same treatment. There may also be problems in the case of generated sources, in which case the source code will be in the build directory, the name and location of which is unknown at the time of writing the cfg file, so that sort of puts us back at square one.

I don't really like the third option, but I thought I'd throw it out there. The second one I find interesting though. Since a staple of modern build systems in the concept of configuring files to generate sources based on the current environment, couldn't it make sense to support that for gcovr.cfg in OOS builds too? What I'd propose is for the logic in find_config_name() to change slightly so that if the source root and the current working directory are different, the current working dir (assumed to be the build root in OOS build) is searched first and if there's no file there, the source root is checked. This way the filter generation would remain the same, and compatibility with generated sources should be maintained.

What do you think?

[latk]

There is a good argument that all filters in a config file should be resolved relative to that config file, but that would make argument handling code tricky – we'd need to either build the filter as part of argument parsing, or annotate every filter expression with its origin to know relative to where it should be interpreted. I'm happy to change how config files work, but the command line arguments must not be affected. You're welcome to work in that direction, if you'd like to.

It is not generally sensible to interpret filters relative to the --root. This is more evident when an explicit --filter is used so that source files outside of the root might be considered. I will also not give the build directory precedence over the root directory. The autodetection is merely a convenience; if a specific location is required then an explicit --config may be specified.

I don't quite understand why an OOS build would make filters difficult in the first place. Given this kind of layout:

project/
  build/
    target-release/
    target-debug/
  src/

And given that gcovr is run from within the target directory, then you can always exclude source files like --exclude '../../src/.*eww\.cpp' – you only need to know the build directory depth and general layout, not its particular path. Since filters are regexes you can specify them quite flexibly, e.g. -e '/.*/src/except-this/' would exclude a particular subdirectory and works with any build mode.

In any case, gcovr's configuration is closely linked to the build system configuration, so I don't think it's reasonable to generally expect the same gcovr configuration to work with wildly different directory layouts.

[kjerstadius]

Thanks for the quick reply!

I may have gotten a bit carried away with my suggestions as, as you rightly point out, the --config option is available.

I think the main gist of the problem is that the build directory location may not be constant. Most of the time I would agree that the build directory location and depth are both known and indeed "below" the sources. However, in some scenarios, such as when using Conan, IME this is not the case. Again though, with a configured config file this need not be a problem.

Anyway, perhaps I'm chasing ghosts here. I agree that the gcovr configuration is closely tied to how the code is supposed to be built. I guess I just dislike the idea of writing a configuration file where I assume the user builds the code in a specific way, or adheres to a specific directory structure. Perhaps getting different coverage results than expected isn't the end of the world, but still.

[latk]

I do not want to make any use cases impossible. If you can explain the directory structure of Conan more clearly, perhaps I would be able to find a solution.

[kjerstadius]

I haven't used Conan that much, but as far as I know, the default behavior is to put the build folder next to the source folder, so it doesn't follow the "build folder beneath source root" paradigm.

Anyway, going back to what you said before about changing the config files, but without affecting command line arguments, how about unconditionally converting filter options from config files into absolute path filters? Since the path to the config file is well known this is almost trivial and can be done when creating ConfigEntry objects. I tested it on my fork and all tests passed. Furthermore, I tried some different regexes on the (admittedly trivial) Meson testcase I've been using and it seems to work as expected.

Let me know what you think.

[latk]

Thank you for experimenting with this stuff! This is actually quite helpful!

Putting the build folder next to the source folder shouldn't be a problem (when using the currently still unreleased development version, which includes some fixes for --root ../$something style invocations.

Filters must not be mangled or manipulated because they are regexes, not paths. Stopping filter transformation was one of the crucial fixes in the gcovr 4.x releases. In particular, filters will break if we have a filter like \.\./foo and prepend an absolute path. The resulting regex /some/path/\.\./foo would be matched by no normalized file path.

I think the problem could be solved by introducing an proto-filter type for argument parsing which contains the filter expression and the context.

• For command line arguments, the context is left empty or set to “.”.
• For config file arguments, the context is dirname(config_file). This would be special-cased in _get_value_fro_config_entry(), similar to how the bool type is special-cased.
• The build_filter() function is extended to use the context rather than the current working directory.

If you'd like to hack on this, that would be most welcome :)

[kjerstadius]

Incidentally that sounds sort of like the "proper" implementation of my first experiment. Now that I'm a bit more familiar with how the argument parsing etc. works I'm happy to give it a go. It might not be until the second half of next week though.

[kjerstadius]

Ok, so I gave this another go based on our discussions, see 73bf9bd. The solution seems to work and passes the test suite.

What I've done is add a class for filter options that contains a field for the regex and a field for the path context. Filter options from config files are created as FilterOption instances as part of the config file parsing. Currently the command line parsing is completely untouched and only inside of build_filter() are the filter options that are "only" strings converted into FilterOption instances.

There are a couple of things I'd like your opinion on:

• Placement of the FilterOption class. I suppose it could be either in configuration.py or utils.py. In the end I chose to put it next to the classes relevant to filters in utils.py. YMMV.
• Handling of command line arguments. Right now the CLI args are treated exactly as before until they're passed to build_filter(). In a way I find this both elegant and a little dirty. It should be quite simple to implement a function that runs over the CLI arguments right after they've been parsed to convert the filter options into FilterOption instances, but I figured I'd ask what you think/prefer before spending time on it.

Cheers

[latk]

Thank you @kjerstadius for that experiment, this is definitely going in the right direction. If you could create a pull request (and add your name to the AUTHORS file) that would make discussion/review of your code much easier. A couple of pointers:

• The FilterOption approach is quite elegant. I don't care where it is placed; the utils package is fine.
• I'd leave the path_context as None or '.' and normalize within build_filter().
• The build_filter() function could/should become a method of FilterOption.
• I'm not a fan of parameters that can have different types (str vs FilterOption).
• In the filter options, we could set type=FilterOption so that command line arguments will be converted to FilterOptions as well. In the _get_value_from_config_entry() function, it might then be better to check if option.type is FilterOption, not whether the option is in the filter-option group. No need to post-process the CLI arguments in a separate pass.

[kjerstadius]

All good points and easy to agree with. Thanks for the feedback. Ironically enough I considered changing the type for the options in question, but I opted for not even trying as I feared it would interfere with the utility provided by check_non_empty(). Clearly I didn't give ArgumentParser enough credit here as it seems to work as one would expect.

Anyway, I'm working on a proper pull request. Shouldn't take more than a day or so.