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

Added ability to strip source file paths prior to opening them. #99

Merged
merged 3 commits into from
Jan 30, 2024

Conversation

lapointejp
Copy link

The goal of this addition is to allow fastcov to analyze notes and coverage files that were generated in a different environment.

One would use the -sp switch to pass-in the absolute path to the project directory where it was built and the resulting source file path would then be a relative one, making exclusion processing work in cross-profiling environments.

@RPGillespie6
Copy link
Owner

RPGillespie6 commented Jan 23, 2024

So if I understand correctly you want to pass:

$ fastcov -sp /some/absolute/path/to/project ...

And have fastcov strip out any substrings that match /absolute/path/to/project from source paths contained in lcov info and fastcov json files before opening them for exclusion marker processing?

Any reason not to just preprocess the fastcov json or lcov info with something like

$ sed 's/\/path\/to\/strip//g' coverage.json > stripped_path_coverage.json

?

I'm also wondering if maybe the better solution is to have a flag that instructs fastcov to write out relative source paths instead of absolute so that the relative path can be controlled by the working directory in which fastcov is invoked.

@lapointejp
Copy link
Author

lapointejp commented Jan 23, 2024

Your interpretation sound about right to me.

In my case, I am building my project with instrumentation. I then invoke it and use fastcov to compile the .info lcov report. I am setting the GCOV_PREFIX_STRIP variable so that gcov uses relative paths and the --sp argument sent to fastcov is allowing fastcov to open the source files based on a relative path as well (which is how the exclusions are taken into account).

If I post-process the .info file for the path, then the exclusions wouldn't be taken into account in the final report.

Pre-processing the .gcno/.gcda files seems like overkill and inefficient when fastcov is already effectively parsing them.

@RPGillespie6
Copy link
Owner

Ah, so maybe the best solution here is to have fastcov respect the GCOV_PREFIX_STRIP and GCOV_PREFIX env variables as well? I think currently fastcov is hardcoded to create an absolute path from the paths inside the intermediate json.

@lapointejp
Copy link
Author

We could also do that - perhaps might feel more "unified". My worry was more on the "undocumented" feature that would bring. I also appreciated the string replacement approach as that would also exclude source files installed on the machine from the exclusion (those in /usr/include for instance).

I'm open to the idea of reacting to the same environment variables, but I personally don't favor their use when I can.

Maybe another approach would be to pass in both of them on the command line and have fastcov set the variables prior to invoking gcov ?

@RPGillespie6
Copy link
Owner

Yeah I like that better. Something like have the flags --gcov-prefix and --gcov-prefix-strip which cause fastcov to set GCOV_PREFIX and GCOV_PREFIX_STRIP under the hood before invoking gcov, but also respect those values later when doing exclusion marker processing. That way the flags are documented in fastcov --help

@lapointejp
Copy link
Author

Sound good. I will amend my changes and update this PR.

@RPGillespie6
Copy link
Owner

My worry with blind string replacement vs prefix is that with programmatic usage you might clobber part of the path unintentionally. For example /tmp/user/source/tmp/x.cpp if you replace /tmp blindly you'll get /user/source/x.cpp when maybe you only intended to strip off the leading /tmp

@lapointejp
Copy link
Author

Good point.

@lapointejp
Copy link
Author

@RPGillespie6, I've updated this PR with the proposed solution. Let me know what you think.

fastcov.py Show resolved Hide resolved
fastcov.py Show resolved Hide resolved
fastcov.py Show resolved Hide resolved
fastcov.py Show resolved Hide resolved
fastcov.py Show resolved Hide resolved
fastcov.py Show resolved Hide resolved
fastcov.py Show resolved Hide resolved
@codecov-commenter
Copy link

Codecov Report

Attention: 6 lines in your changes are missing coverage. Please review.

Comparison is base (9595b3e) 100.00% compared to head (1f70f01) 99.17%.
Report is 5 commits behind head on master.

Files Patch % Lines
fastcov.py 88.67% 6 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@             Coverage Diff             @@
##            master      #99      +/-   ##
===========================================
- Coverage   100.00%   99.17%   -0.83%     
===========================================
  Files            1        1              
  Lines          694      731      +37     
===========================================
+ Hits           694      725      +31     
- Misses           0        6       +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@RPGillespie6 RPGillespie6 merged commit c80ad2b into RPGillespie6:master Jan 30, 2024
1 of 3 checks passed
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.

3 participants