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

Fix --exclude-directories option #266

Merged
merged 5 commits into from Jul 2, 2018

Conversation

Projects
None yet
3 participants
@mkurdej
Contributor

mkurdej commented Jun 25, 2018

  • Add test for --exclude-directories.
  • Add test for --exclude.
  • Use match method instead of search in Filters (link_walker).
@latk

This comment has been minimized.

Member

latk commented Jun 25, 2018

Thank you for trying to fix this, I'll take a closer look during the next few days. Btw you forgot to commit one of the file1.cpp files which causes the tests to fail?

TBH I don't really know what the --exclude-directories is supposed to do. I think it was intended as an optimization when searching for gcda/gcno files, but for some reason it seems to work differently than most filters (being matched against a directory name instead of a full path). I'm pretty sure it was broken for the last few months, but so far no one has complained.

Are you using or trying to use this option in your projects? If so, may I ask how? I'd like to tweak the behaviour to be actually useful :)

@codecov

This comment has been minimized.

codecov bot commented Jun 25, 2018

Codecov Report

Merging #266 into master will increase coverage by 0.61%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #266      +/-   ##
==========================================
+ Coverage   89.77%   90.39%   +0.61%     
==========================================
  Files          13       13              
  Lines        1555     1551       -4     
  Branches      271      268       -3     
==========================================
+ Hits         1396     1402       +6     
+ Misses        105       98       -7     
+ Partials       54       51       -3
Impacted Files Coverage Δ
gcovr/utils.py 70.74% <100%> (+1.99%) ⬆️
gcovr/gcov.py 83.88% <0%> (+0.83%) ⬆️
gcovr/html_generator.py 97.4% <0%> (+0.86%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3deff34...87a8a2c. Read the comment docs.

@mkurdej

This comment has been minimized.

Contributor

mkurdej commented Jun 25, 2018

Hi @latk, I've just added the missing file. Sorry for the omission.
So, for --exclude-directories, we do use it in our projects. The use case is simple, we exclude coverage of test files in some reports.
So, it's a bit different than excluding test files altogether since we still use these gcda/gcno files in other reports.
I can imagine that this option is not widely used, especially that the name seems a bit misleading (there is no symmetry between --exclude (for source files) and --exclude-directories (for gcda/gcno files)).

BTW, it had been working fine for a long time, but we updated gcovr to 4.0 version a few days ago and that's where the issue came out.

@latk

This comment has been minimized.

Member

latk commented Jun 25, 2018

Ok, so I took a deeper look.Sorry, this is a bit long :)

(1) Can you add your name to the AUTHORS file?

(2) Please feel free to rebase and force-push freely on this PR. I'll probably do some rebasing and squashing of fixup commits anyway when I merge your work.

(3) I see that you tried to test for an empty --exclude-directories option but ultimately removed the test (because gcovr currently doesn't check for that?). I think it would be good to have that test for --exclude-directories AND --exclude because an empty filter would exclude everything. This would require the addition of a check near the top of the main() function, similar to the check for the empty --root option.

Would you like to do that, possibly as a separate PR?

(4) Your --exclude test looks good :)

(5) In the link_walker() function, I assume some further changes might be necessary:

     for root, dirs, files in os.walk(os.path.abspath(path), followlinks=True):
         for exc in exclude_dirs:
+            dirs[:] = [d for d in dirs if not exc.match(os.path.join(root, d))]
-            for d in dirs:
-                m = exc.match(d)
-                if m is not None:
-                    dirs[:] = [d for d in dirs if d is not m.group()]
         yield (os.path.abspath(os.path.realpath(root)), dirs, files)

or something like that. I.e. remove the d is not m.group() identity test and just check for match, and match against the full path instead of just the directory name so that the filter works correctly. Possibly it would also make sense to turn the loops over dirs and exclude_dirs inside out:

    [d for d in dirs if not any(exc.match(...) for exc in exclude_dirs)]

(6) In the --exclude-directories test as currently written, nothing is currently excluded. I think ideally the build process is changed so that the object files are placed into a directory structure like

build/
  x/file1.o
  y/main.o

Then we can test whether gcovr --exclude-directories x would correctly exclude the file1.gcno/file1.gcda files that should appear in that directory. My guess is that this will not work because of the changed filtering system, and the correct pattern would now be build/x/. This could be combined into the command line gcovr --exclude-directories build/x/ --exclude-directories y where we expect that y does not match.


What are your thoughts on the above ideas? I'm not sure whether they are sensible and/or will work.

Thank you very very much for submitting these test cases. The link_walker() function is one of the remaining areas I still don't quite understand since I took over maintenance for gcovr, so finally putting them under test is very helpful.

I assume that the --exclude-directories option was broken since gcovr 3.4 (published Feb 2018) in a commit that made the other filters work under Windows. The breakage probably became more noticeable in gcovr 4.0 when the filtering system was redesigned. I'm sorry that this caused any inconvenience on your end. I'll publish a bugfix release when this is resolved.

@mkurdej

This comment has been minimized.

Contributor

mkurdej commented Jun 26, 2018

Hey, thanks for all the suggestions.
I've done most of what you proposed.
I'll use 2) later, but I imagine you could just squash everything.
I'll do another PR for 3).
For 6), using gcovr --exclude-directories build/a --exclude-directories b actually excludes both a and b. I'm not sure what should be the correct behaviour though. I only excluded build/a (without trailing slash BTW) in committed tests.
What are your thoughts on that?

@mkurdej

This comment has been minimized.

Contributor

mkurdej commented Jun 26, 2018

Oh, and I might have done some minor modifications (renaming variables) in between the commits. I may remove them or do a separate commit/PR if you want a clean PR.

@totoc1001

This comment has been minimized.

totoc1001 commented Jul 2, 2018

Thanks for this fix, I'm unfortunately trying to go through the same problem,
I've got a lot of gcda / gcno files generated in a very big project.
I really need to be able to avoid generating coverage files for some directories otherwise it will be very long to perform... (around hours...)

I don't know whether it's planned or not, but may I suggest also updating the doc about this option?

Thanks a lot for your help.
Thomas

@latk latk force-pushed the mkurdej:fix-exclude-directories branch from 21b7de0 to 5274619 Jul 2, 2018

@latk latk force-pushed the mkurdej:fix-exclude-directories branch from 5274619 to 87a8a2c Jul 2, 2018

@latk

latk approved these changes Jul 2, 2018

@latk latk merged commit 87a8a2c into gcovr:master Jul 2, 2018

4 checks passed

codecov/patch 100% of diff hit (target 89.77%)
Details
codecov/project 90.39% (+0.61%) compared to 3deff34
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@latk

This comment has been minimized.

Member

latk commented Jul 2, 2018

Thank you @mkurdej for working on this, I've merged the changes and have published them as the gcovr 4.1 release.

If you'd still like to write those --exclude/--exclude-directory validations + tests, I would of course be delighted :)

@latk

This comment has been minimized.

Member

latk commented Jul 2, 2018

@totoc1001 I'm sorry to hear that gcovr is slow for you. I've made a new release with the fixed --exclude-directories option so you can update and use it now!

I've also added a section to the docs about the behaviour of this option and various interactions. E.g. you might also want to try out the -j option that turns on parallel execution, but it doesn't always help.

There is one big efficiency problem remaining: gcovr doesn't know which coverage data files belong to which source files, and runs the gcov tool in various directories until something works. The code for that is in gcovr.gcov.process_datafile(). I don't really understand this code, or why it is necessary.

If this is costing you hours, you might want to investigate how it works and whether it can be optimized. I'd be really glad to have a more efficient solution.

As a last resort, consider running gcov yourself to produce the .gcov files, and then invoke gcovr with the --use-gcov-files option. This is a bit more fragile because you'll have to pick the right gcov options, but it can potentially be a lot faster.

I hope this helps!

@totoc1001

This comment has been minimized.

totoc1001 commented Jul 2, 2018

@mkurdej mkurdej deleted the mkurdej:fix-exclude-directories branch Jul 3, 2018

@mkurdej mkurdej restored the mkurdej:fix-exclude-directories branch Jul 3, 2018

@mkurdej mkurdej deleted the mkurdej:fix-exclude-directories branch Jul 3, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment