Python 3.8 breaks network drive setups (#365) #535
Conversation
There was a problem hiding this comment.
I understand the problem but I want to discuss the design in general. We have also #420 where the inconsitent handling.
What about defining a option --path_handling=legacy(default)|resolve_symlinks|keep_symlinks?. Depending on the option a class to handle paths is configured and this class is used instead of the direct calls to realpath and abspath.
I also prefer to configure a testcase first which is only used on Windows clients. This testcase can directly use subst to a local directory. This testcase shall fail on python 3.8 and the code change shall fix the test.
One additional hint, if you use the network dialog or net use X: \\computer\share is not so good for the build performance, it is better to use subst X: C:\path\to\dir because this bypasses the network adapter.
|
I've updated the changelog. |
Codecov Report
@@ Coverage Diff @@
## master #535 +/- ##
==========================================
- Coverage 95.70% 94.98% -0.72%
==========================================
Files 23 21 -2
Lines 3280 2872 -408
Branches 603 542 -61
==========================================
- Hits 3139 2728 -411
- Misses 69 76 +7
+ Partials 72 68 -4
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
|
In the review comment #535 (comment), the issue of mentioning companies in the Authors file arose. I think this is perfectly fine. The authors file is not an exhaustive list of copyright holders. It is more about honouring and crediting anyone who contributed to gcovr, and hinting at potential copyright holders. Out of respect of privacy, this is completely optional and pseudonyms are welcome. In general, I have nothing against companies being listed as contributors, as long as it comes with no strings attached (e.g. there will never be URLs in this file). There are also some “evil” companies where I'd rather miss out on the contribution than acknowledge them. Even if Verequus is not contributing on their own behalf, I'd think individual attribution would be appropriate in addition to attributing the company as the copyright holder. Do we need a written form? I don't think so. I've been happy that gcovr is low-bureaucracy, with no Developer Certificate of Origin or similar. Contributions explicitly from a company are arguably even lower-risk than contributions from employees who might not know whether they're authorized. My instinct would be to keep it the way it has been, but of course to remain wary of potential impersonations or unauthorized use of names. I will act in a good faith belief that all contributions are OK, and have my contact details in the package metadata in case there should be issues. |
| if use_canonical_paths: | ||
| fname = os.path.realpath(fname) | ||
|
|
There was a problem hiding this comment.
Often, fname will already be a canonical path. I think it would be correct to always use fname = os.path.realpath(fname) here. Removing such conditionals simplifies testing.
There was a problem hiding this comment.
Shall I change the code appropriately then?
There was a problem hiding this comment.
I've checked the surrounding code. I think applying an unconditional realpath() in that location is neither harmful nor necessary. So I'd leave it in, and see if any tests fail. But there are many uses of normpath() or abspath() that should probably be audited.
There was a problem hiding this comment.
Without this line, gcovr failed to match the files correctly in my environment. So removing it makes no sense. I'll turn it unconditional instead.
There was a problem hiding this comment.
@latk In my tests with substitutions the test filter-relative-lib-from-unfiltered-tracefile fails with this. I think the test data is incorrect here because the filters for the tracefile are:
Filters for --root: (1)
- re.compile('^\\/gcovr\\/gcovr\\/tests\\/filter\\-relative\\-lib\\-from\\-unfiltered\\-tracefile\\/project\\/')
Filters for --filter: (1)
- DirectoryPrefixFilter(\/gcovr\/gcovr\/tests\/filter\-relative\-lib\-from\-unfiltered\-tracefile\/project\/)
and the file yes.cpp is filtered out in the tracefile:
fname /gcovr/gcovr/tests/filter-relative-lib-from-unfiltered-tracefile/external-library/src/yes.cpp
Parsing coverage data for file /gcovr/gcovr/tests/filter-relative-lib-from-unfiltered-tracefile/external-library/src/yes.cpp
Filtering coverage data for file /gcovr/gcovr/tests/filter-relative-lib-from-unfiltered-tracefile/external-library/src/yes.cpp
There was a problem hiding this comment.
Indeed, the test seems incorrect. In the gcovr invocation that creates the JSON file, I think there should be a --root .. so that the libraries aren't prematurely excluded. No idea what we were thinking back then. I'm glad the work on this PR has discovered that bug.
There was a problem hiding this comment.
I've checked the issue again and it seems that it came from to much realpath calls. So nothing to do at the moment.
|
For me the agreement of @Verequus is also enough. I was asking because my company is afraid of recourse claims if the company name is used. |
I think it is important to avoid adding more and more options wherever possible. If there is a clearly correct behaviour, it should always be chosen and no option is needed. With this PR, I am not confident that I know what is and isn't correct. My hunch is that this change is always correct on Windows, and that the previous behaviour was buggy – but if that were the case then the test related to symlinks would maybe start passing on Windows… Before we add an option that makes path treatment highly configurable, I'd like to be convinced that this configurability is needed. Ideally, a test that shows that desirable behaviour on Windows would be impossible with this option. For example the discussion in #365 mentions potential permission problems. If this configurability is needed, then offering a choice between realpath/abspath mode could make sense. Not sure about the naming. I'd guess As suggested, a class that handles path munging would be sensible so that all occurrences of abspath/realpath/relpath can be audited and harmonized. But only if the configurability is needed. |
|
The idea of
I think, using I'll try to extend the test for windows to get the errors of #365. From there we can implement our own |
Co-authored-by: Michael Förderer <michael.foerderer@gmx.de>
* Replacing duplicated code with singular function * Removing incorrect code
|
Hi, In regards to #420, I'm not sure what do myself and how this interacts with the proposed code. @Spacetown So you are going to add the missing UTs? Or should I still try to add some tests? Otherwise, I've updated the code according to the feedback given so far. |
|
@Verequus pleas wait with changes from your site. |
Co-authored-by: Lukas Atkinson <opensource@LukasAtkinson.de>
|
I'm working since two days on my branch https://github.com/Spacetown/gcovr/tree/add_test_drive_subst_windows but I don't get Unix and Windows running. One problem are the filters because not all paths are resolved (https://github.com/Spacetown/gcovr/actions/runs/1604044188). |
|
@Spacetown As in without my own modifications? |
Yes, only this branch. |
|
I run again into the error "Paths don't have the same drive", which suggests that my change is still necessary. |
|
And what about the branch Spacetown:add_test_drive_subst_windows_1? |
|
Have you installed virtualenv (see https://mothergeo-py.readthedocs.io/en/latest/development/how-to/venv-win.html)? |
|
I had virtualenv installed already, but I only get this output:
|
|
Can you try to remove the virtualenv folder? Maybe there was an error setting up the virtualenv. |
|
Where do I find that folder? I didn't create one manually for the tests since I couldn't figure out how to make the tests use that one. |
Co-authored-by: Michael Förderer <michael.foerderer@gmx.de>
* Replacing duplicated code with singular function * Removing incorrect code
Co-authored-by: Lukas Atkinson <opensource@LukasAtkinson.de>
Discovered by calling the module normally
|
As a colleague did some testing using a different branch, he discovered that my changes here were insufficient. So I updated that branch here with the findings. |
|
Here is the full output: |
|
The log file suggests that your current working directory with the gcovr repository contains a You mentioned using WSL. You must decide whether you're working on gcovr via Linux/Unix (in which case WSL provides a suitable environment) or via Windows. Our QA tooling does not expect that you switch back and forth between Windows and WSL. For example, if you ran You cannot run the Windows-specific tests from WSL. But running the tests on native Windows requires installing Unix-ish build tools on Windows, in particular GNU Make, Bash, and one of the supported compilers (GCC 5/6/8 or Clang 10). Our test suite does this via MSYS2. It is not possible to use programs installed via WSL for the Windows tests. |
|
That's the reason why we are using different environment directory for nox in docker. Can you also post the full log of this error?
|
|
@latk It was indeed the fact I've run it under WSL before. But I was too busy with the logging updates to download MSYS2 today. Will look at this next week. @Spacetown Do you mean the unredacted lines? Or the whole log? Both things contain project-related information which I do not wish to make public. It should be possible to organize something over a private channel, but that would require some sort of NDA. |
|
@Verequus starting with the traceback of the exception will be enough. |
|
@Spacetown The traceback occurs inside project-related code, since we call directly into the plugin's code. So it doesn't help. What is clear, that the data retrieved from the gcovr code has the odd mixture of paths for whatever reason. |
Ah, that's interesting. I found one special treatment for windows in our code: Lines 259 to 263 in b6e1850 |
|
When I reworked the code to use the Filter classes from gcovr (they were previously not useful because the OS-independent path separator replacement was not done at the root of the classes - I did move that modification into gcovr core, but for whatever reason the current master works without any further changes for the Filters), I noticed that my project code employs the AbsoluteFilters for the current branches. While a LTR branch still calls the module as currently intended and due wrong arguments it was not noticed that a number of paths required use_canonical_paths in addition to the changes at the beginning. Those particular paths were related to the use of RelativeFilter. I can't help but notice that your specific code only exists in RelativeFilter. That means that AbsoluteFilter is likely not used in your test (haven't looked into it yet). |
|
The |
|
@Verequus I think this special check isn't needed because the filter is based on a RegEx. For me the stack backtrace is essential because the message you showed isn't the place of the error. I need the call chain to analyze it. |
|
@Spacetown I don't know what to tell you. The blowup happens at a point of time, where no gcovr code is no longer active (in other words, the stacktrace shows no gcovr functions running) in that line:
This for the simple reason that the path given by gcovr still uses Y: and not C:. Changing our code to use not realpath doesn't work either, since there are two different folder trees where the code lives and for the second one the gcovr code does translate it from Y: to C: (I'd need to recheck but if this wouldn't be true, I would have not attempted to modify gcovr). It didn't occur to me until now that I could do the realpath again in that line in our code, which most likely will solve that issue. But that doesn't change the fact that even inside of gcovr the translation happens only partially. Even if one could explain currdir with possibly having "realpath()"-ed the input folder already, how end up gcov_fname and source_fname being different?
I'll do a test without realpathing the input parameters in advance, then it should be clear if that remains a problem as I believe. |
|
Ok, now it's clear for me why I can't reproduce the problem. |
|
Since I'll be leaving my company, I won't be able to keep testing stuff in the project environment. Instead my colleague bmazilu will hopefully be able to continue the work. I'll try to support from the sidelines, if possible, nonetheless. |
|
Since the usage of realpath was changed with #568 and the test with drive substitution is OK I'll close this request. |
Python 3.8 breaks network drive setups (#365) contains my motivation. I've chosen this approach to solve the issue - hiding the change behind an option, since I can't test without the change on Windows nor on Linux. This means, that the patch might be possibly more general applicable than I conservatively can risk. I will investigate if I can add a unit test and update accordingly. But for now I'd like to have some feedback if there are greater changes needed.
Fixes #365