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

Remove dead path handling code #280

merged 6 commits into from Sep 28, 2018

Remove dead path handling code #280

merged 6 commits into from Sep 28, 2018


Copy link

@latk latk commented Sep 16, 2018

This PR removes some path handling code that seems to be dead code. But “I have only proved it correct, not tried it.” Please test on real-world projects.

Summary of changes:

  • The PathAliaser class was removed, as its abilities are not being used.

  • Some path-related standard library workarounds could be removed, since we are targeting Python 2.7 or better. The notable exception is the commonpath() function which only entered the standard library in Python 3.5 – and our implementation has some subtle differences. At least we now match the CPython standard library implementation more closely.

  • Behaviour of --object-directory was removed that could only be executed when the objdir was a .gcda file. This was undocumented, untested, and is extremely unlikely to have been used in practical projects. However, see the next point.

  • Behaviour of --object-directory was removed that tries to deal with gcc -o ../build style compiler invocations. The previous code was definitively incorrect. For details see the commit messages, here's the TL;DR:

    The purpose of --object-directory is to infer the working directory where gcc was invoked, based on the path of a .gcda file. This working directory is needed so that gcov can find the source files. E.g. if gcc -o ../build/foo.o foo.c was invoked from /project/src, then we would get the coverage data /project/build/foo.gcda.

    The idea of the previous code was to walk the path from the gcda file backwards. To deal with the .. segment we would glob all possible directories, here leading to /project/* as potential working directories. But as implemented it would only work if the --object-directory was gcc's path to the .gcda file, i.e. ../build/foo.gcda.

    The --object-directory had two fallback heuristics that are kept by this PR. In particular:

    • if the objdir exists (as an absolute or relative path), it is used as a working directory suggestion. This is the only reasonable interpretation, although the option name is now totally confusing.
    • if the objdir is relative, it is also appended to the gcda directory (in the hope that it is a reverse path?). In this example we would get /project/build/../build which doesn't make a lot of sense.

    Note that the nested3 test case uses a relative --object-directory to point to the build output directory, while invoking GCC from the root directory. I now believe this usage is incorrect and only works in that test because the --root is added unconditionally as a working directory suggestion. E.g. the test fails if the compiler is invoked from multiple directories.
    ↑ future work

I believe that unclear objdir semantics might be related to the occasional “gcovr doesn't work with CMake” reports. This PR has no fix, but I now have a clearer understanding of this problem.

latk added 6 commits Sep 16, 2018
This commit removes unreachable/unnecessary link handling code. In

  - os.path.realpath() already handles links properly, so no custom
    implementation is needed.

  - The PathAliaser capabilities are not exposed to users and can
    therefore be removed. This code could be removed because its caches
    were never written, only read from.

I assume this code was originally written for Python versions that did
not have sufficient support for symlinks? However, that is no longer an
lift the conditional and loop out of the function into the main,
use fewer magic numbers.
The find_potential_working_directories_via_objdir() function has various
heuristics, and some seem to be dead code. The removed code had the
following behaviour:

    let abs_filename be the absolute .gcda file name. Examples:
        (f1) "/the-project/build/foo.gcda"
        (f2) "/the-project/build/something/foo.gcda"

    let objdir be the object directory. Examples:
        (o1) "/the-project/build/"
        (o2) "../build/different/../"
        (o3) "/the-project/build/foo.gcda"
        (o4) "../build/foo.gcda"
        (o5) "../.."

    The objdir is normpath()'d, which folds parent directory segments
    away. Any remaining ".." segments will be at the start.
        (o1) "/the-project/build"
        (o2) "../build"
        (o3) "/the-project/build/foo.gcda"
        (o4) "../build/foo.gcda"
        (o5) "../.."

    Common trailing path segments are removed from both paths.
    This will only do anything if objdir points to that specific
    gcda file. Examples:
        (o1)    (unchanged)
        (o2)    (unchanged)
        (o3,f1) "" (paths are equal)
        (o3,f2) o:"/the-project/build",
                (basename is common)
        (o4,f1) o:".."
        (o4,f2) o:"../build"
                (basename is common)
        (o5)    (unchanged)

    If the objdir was completely matched, return no working dirs.
    This applies in case (o3,f1).

    If the objdir ends with a ".." segment (which can only happen if all
    remaining segments are ".." due to the normpath()), expand the
    remaining file names like a "*/" glob pattern (but always strip one
    extra segment).
        (o4,f1) glob "/*/"
        (o5,f1) glob "/the-project/build/*/*/"
        (o5,f2) glob "/the-project/build/something/*/*/"

Since the objdir should be a directory and not a single gcda file, the
(o3) and (o4) can never be encountered in practice.

The (o5) case (objdir only consists of ".." segments) could be observed
in practice, but would indicate an extremely weird build process. The
resulting glob patterns have no reasonable interpretation and are
unlikely to match a good working directory.

The remaining heuristics can be matched and are left intact.

    If the objdir is absolute: If the objdir exists it is returned as a
    working directory suggestion. This matches (o1) and (o3).

    If the objdir is relative, it is appended to a base path. As a base
    path, the CWD and the dirname(abs_filename) are tried. The following
    paths would be tested, any existing ones will be returned:
        (o2,f1)  "/the-project/build/../build/different/../"
        (o2,f2)  "/the-project/build/something/../build/different/../"
        (o2,cwd) "../build/different/../"
        (o4,f1)  "/the-project/build/../build/"
        (o4,f2)  "/the-project/build/something/../build/"
        (o4,cwd) "../build/"
    Some of these suggestions are unlikely to exist, e.g. the (f2) cases
    where the gcda is in a subdirectory.

The only reasonable interpretation is that objdir might be intended as
an unmodified path, but the other cases will have to stay for now.
@latk latk merged commit 8f15556 into gcovr:master Sep 28, 2018
3 of 4 checks passed
Copy link

@codecov codecov bot commented Sep 28, 2018

Codecov Report

Merging #280 into master will increase coverage by 2.58%.
The diff coverage is 79.74%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #280      +/-   ##
+ Coverage   91.64%   94.22%   +2.58%     
  Files          14       14              
  Lines        1591     1507      -84     
  Branches      281      260      -21     
- Hits         1458     1420      -38     
+ Misses         83       43      -40     
+ Partials       50       44       -6
Impacted Files Coverage Δ
gcovr/ 94.44% <100%> (+0.22%) ⬆️
gcovr/ 91.05% <68.18%> (+14.46%) ⬆️
gcovr/ 86.6% <80.85%> (+2.71%) ⬆️

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 824a83d...8f15556. Read the comment docs.

@latk latk deleted the remove-dead-paths branch Jan 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
None yet

Successfully merging this pull request may close these issues.

None yet

1 participant