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

fix wrong details html filename in windows #375

Merged
merged 10 commits into from Jul 16, 2020
Merged

Conversation

sgheppy
Copy link
Contributor

@sgheppy sgheppy commented May 14, 2020

in windows html details files created like sysrcfile.C:_restofpath.html

@codecov
Copy link

codecov bot commented May 14, 2020

Codecov Report

Merging #375 into master will increase coverage by 0.28%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #375      +/-   ##
==========================================
+ Coverage   94.66%   94.94%   +0.28%     
==========================================
  Files          18       19       +1     
  Lines        2154     2157       +3     
  Branches      373      372       -1     
==========================================
+ Hits         2039     2048       +9     
+ Misses         54       50       -4     
+ Partials       61       59       -2     
Impacted Files Coverage Δ
gcovr/html_generator.py 98.19% <100.00%> (+2.52%) ⬆️
gcovr/tests/test_html_generator.py 100.00% <100.00%> (ø)

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 39f5a82...9a38ff9. Read the comment docs.

@latk
Copy link
Member

latk commented May 27, 2020

Thanks for finding and fixing this issue! Cross platform path handling is awful 😂

Just two potential enhancements:

  • would you like to add your name to the AUTHORS file?
  • could you add a comment near your change to explain for future readers why escaping : is necessary? Usually it would be better to create a test case, but here that might be difficult.

@sgheppy
Copy link
Contributor Author

sgheppy commented May 29, 2020

Thanks for finding and fixing this issue! Cross platform path handling is awful 😂

Just two potential enhancements:

  • would you like to add your name to the AUTHORS file?
  • could you add a comment near your change to explain for future readers why escaping : is necessary? Usually it would be better to create a test case, but here that might be difficult.

When using gcovr in windows the _make_short_sourcename function in html_generator module return something like that:
C:\<path>\gcovr_files\c_coverage.C:_<path>_<sourcefile>.c.html.
I don't know with no error is generated when windows create file with this path but that file is create and readable by browser but isn't reachable by file managers.

I'll implement a test case for that.

Thank you for great tool.

@Spacetown
Copy link
Member

Spacetown commented May 31, 2020

I don't know with no error is generated when windows create file with this path but that file is create and readable by browser but isn't reachable by file managers.

You can't create a file with a : in windows, see Naming Files.

Copy link
Member

@latk latk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for updating the PR! I like that you're testing for relevant properties for the output, instead of asserting that the result is a specific string. But what do you think of rephrasing the assert as suggested below?

gcovr/tests/test_html_generator.py Outdated Show resolved Hide resolved
gcovr/tests/test_html_generator.py Outdated Show resolved Hide resolved
@sgheppy
Copy link
Contributor Author

sgheppy commented Jun 2, 2020

I' found another error, in case of using combination of tracefiles feature the value of parameter a filename is like /dir1/dir2/source.c in windows too.
The char '/' is not escape in windows.
Can I add that fix in this PR?

@latk
Copy link
Member

latk commented Jun 2, 2020

Oh, good catch! Yes, you can update the PR to fix this as well.

@@ -373,6 +376,7 @@ def _make_short_sourcename(output_file, filename):
# filename if it exceeds common filesystem limitations
if len(os.path.basename(sourcename)) < 256:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The limit is 260 characters (MAX_PATH).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for pointing.
Maybe it's needed to consider all path and not only sourcename.
What do you think?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All paths should respect this limit. What about using only the basename of the file extended by the md5sum of the whole path e.g.:
path/to/source.c --> source.c.883f9ee88c14d9c77addd640015ee77d.html

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is already that:

        longname_hash = "_" + hex(zlib.crc32(longname.encode('utf-8')) & 0xffffffff)[2:]
        longname = longname[(len(sourcename) - len(longname_hash)):]

Isn't good enough?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggested to always use the naming schema instead of removing the known bad characters and use a hash if the name gets to long.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I didn't undestand that before.
I'm agree with you, i think is the best approach to avoid path errors.

Would you like me to do it?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think @latk should decide this. I also prefer md5 instead of crc32.

Copy link
Member

@Spacetown Spacetown left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems to be easier and robuster than removing known bad characters.

@sgheppy
Copy link
Contributor Author

sgheppy commented Jun 16, 2020

Seems to be easier and robuster than removing known bad characters.

but a lot of tests fail, as soon as possible i try to fix those fails

@Spacetown
Copy link
Member

Seems to be easier and robuster than removing known bad characters.

but a lot of tests fail, as soon as possible i try to fix those fails

You can use the opion ``

Seems to be easier and robuster than removing known bad characters.

but a lot of tests fail, as soon as possible i try to fix those fails

You can use the option TEST_OPTS=--generate_reference and remove the old file if the content is equal. The main file can be updated with TEST_OPTS=--update_reference.

@sgheppy
Copy link
Contributor Author

sgheppy commented Jun 19, 2020

sorry but i don't understand.
if i type

CC=gcc CXX=g++ GCOV=gcov PYTHON=python TEST_OPTS=--generate_reference make test
I've got that error:

__main__.py: error: unrecognized arguments: --generate_reference
  inifile: None
  rootdir: c:\Users\antonio\Documents\GitHub\gcovr```

@Spacetown
Copy link
Member

Rebase your branch first. This option is new. In old version you have to modify the init value in the script.

@sgheppy
Copy link
Contributor Author

sgheppy commented Jun 27, 2020

Seems to be easier and robuster than removing known bad characters.

but a lot of tests fail, as soon as possible i try to fix those fails

You can use the opion ``

Seems to be easier and robuster than removing known bad characters.

but a lot of tests fail, as soon as possible i try to fix those fails

You can use the option TEST_OPTS=--generate_reference and remove the old file if the content is equal. The main file can be updated with TEST_OPTS=--update_reference.

I think that create difficult for testing.
I think the md5 calculate are different for windows case and for linux case due to the different path strings.

>               assert diff_is_empty, "Unified diff output:\n" + "".join(diff_out)
E               AssertionError: Unified diff output:
E                 --- reference/coverage.html
E                 +++ coverage.html
E                 @@ -73,7 +73,7 @@
E                  
E                    <tr>
E                      <th scope="row">
E                 -      <a href="coverage.lib.cpp.490ae91831951467a12a1c7ec793778b.html">lib/lib.cpp</a>
E                 +      <a href="coverage.lib.cpp.fe0bf7776d5f70ffd16bbbe1585c3b26.html">lib/lib.cpp</a>
E                      </th>
E                      <td>
E                        <meter class="coverage-medium" min="0" max="100" value="80.0" title="%">80.0</meter>
E                 @@ -87,7 +87,7 @@
E                  
E                    <tr>
E                      <th scope="row">
E                 -      <a href="coverage.tmp.cpp.0c65fbd45fcccd24c0a02610fc16cf47.html">testApp/tmp.cpp</a>
E                 +      <a href="coverage.tmp.cpp.5862a73542fa66ffe4756306b76b28bb.html">testApp/tmp.cpp</a>
E                      </th>
E                      <td>
E                        <meter class="coverage-medium" min="0" max="100" value="80.0" title="%">80.0</meter>
E                 
E               assert False

@Spacetown
Copy link
Member

What about normalizing? Make relative to root and change path separateor to slash?

@sgheppy sgheppy force-pushed the master branch 2 times, most recently from c8a8723 to 2b3c22a Compare July 12, 2020 17:26
@sgheppy
Copy link
Contributor Author

sgheppy commented Jul 14, 2020

Is that ok?

@Spacetown
Copy link
Member

I'll try to check it this week.

@Spacetown Spacetown merged commit 7b383a6 into gcovr:master Jul 16, 2020
@Spacetown
Copy link
Member

Thanks for this fix.

@sgheppy
Copy link
Contributor Author

sgheppy commented Jul 16, 2020

Thank you for your patience

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

Successfully merging this pull request may close these issues.

None yet

3 participants