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

File in sub-dir of staticdir in long-path notation would not load #1638

Merged
merged 1 commit into from Oct 15, 2017
Merged

File in sub-dir of staticdir in long-path notation would not load #1638

merged 1 commit into from Oct 15, 2017

Conversation

@Safihre
Copy link
Contributor

@Safihre Safihre commented Oct 10, 2017

If a staticdir is supplied in Windows long-path notation, serving a file from a subdirectory would fail because os.stat does not like the mixing of forward and backwards slashes.
Tested on both Python 2.7.14 and 3.6.2.

In the included test the old behavior would be to do:

os.stat('\\?\C:\cherrypy\static/index.html')

Which fails due to the mixed slashes, resulting in a 404.

@codecov
Copy link

@codecov codecov bot commented Oct 10, 2017

Codecov Report

Merging #1638 into master will decrease coverage by 0.1%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1638      +/-   ##
==========================================
- Coverage   77.06%   76.96%   -0.11%     
==========================================
  Files         106      106              
  Lines       14299    14306       +7     
==========================================
- Hits        11020    11010      -10     
- Misses       3279     3296      +17
Copy link
Member

@webknjaz webknjaz left a comment

Could you please deal with reduced coverage?

@@ -314,6 +314,10 @@ def staticdir(section, dir, root='', match='', content_types=None, index='',
branch = request.path_info[len(section) + 1:]
branch = urllib.parse.unquote(branch.lstrip(r'\/'))

# On Windows, correct the path for long-path support
if os.name == 'nt':
branch = branch.replace('/', '\\')

This comment has been minimized.

@webknjaz

webknjaz Oct 11, 2017
Member

I think it's better to start relying on pathlib

This comment has been minimized.

@Safihre

Safihre Oct 11, 2017
Author Contributor

Pathlib is not available in Python 2.7 or anything below 3.4 so I don't think it applies right now.

This comment has been minimized.

@Safihre

Safihre Oct 11, 2017
Author Contributor

You want to introduce a whole new dependency just for a 2 line change?

This comment has been minimized.

@webknjaz

webknjaz Oct 11, 2017
Member

Well, this just looks redundant to me, that's why I'm looking into using this package, which handles path processing way better than such simplistic things.

And I don't truly understand what is this \\? notation and why one would like to combine different delimiter styles. I just want this change, which I cannot check, to not reduce reliability.

This comment has been minimized.

@Safihre

Safihre Oct 11, 2017
Author Contributor

Agreed, Windows is weird. Only using this notation we can reliably get all paths (including those with unicode) to work on Windows. The mixing of delimiter styles is inherent to Cherrypy running on Windows, in that sense it's actually surprising that it worked so far. Windows users will supply paths in Windows style.
Unfortunately I have no experience with pathlib and combined with my lack of understanding of cherrypy internals, me implementing it would probably not work out well.
Simple isn't necessarily bad ;)

This comment has been minimized.

@webknjaz

webknjaz Oct 11, 2017
Member

Okay, let's postpone pathlib integration question, but I'll ask you to document the problem this change solves in code via comment.

@@ -132,6 +132,14 @@ def dynamic(self):
'error_page.404': error_page_404,
}
}

# Windows specific long-path support
if(platform.system() == 'Windows'):

This comment has been minimized.

@webknjaz

webknjaz Oct 11, 2017
Member

Don't use parens.

This comment has been minimized.

@webknjaz

webknjaz Oct 11, 2017
Member

Plus this doesn't need to be conditional.

if(platform.system() == 'Windows'):
rootconf['/static-long'] = {
'tools.staticdir.on': True,
'tools.staticdir.dir': '\\\\?\\%s' % curdir,

This comment has been minimized.

@webknjaz

webknjaz Oct 11, 2017
Member

Why don't you use r'string' notation here?

@Safihre
Copy link
Contributor Author

@Safihre Safihre commented Oct 11, 2017

What can I do about the reduced coverage? Have you looked at the report?
It says reduced coverage because it's platform specific and probably codecov is not testing the Windows specific stuff.

@webknjaz
Copy link
Member

@webknjaz webknjaz commented Oct 11, 2017

@Safihre It looks like codecov's CI detection script has broke:
https://ci.appveyor.com/project/jaraco/cherrypy/build/1.0.330/job/wljtenhosjpuqhjd#L884

@Safihre
Copy link
Contributor Author

@Safihre Safihre commented Oct 11, 2017

That explains.
Probably because it's building a PR and thus doesn't include secret-vars and thus the key is missing.

@webknjaz
Copy link
Member

@webknjaz webknjaz commented Oct 11, 2017

@Safihre it's not related to PR. codecov works w/o tokens for public builds in public CIs

@cherrypy cherrypy deleted a comment Oct 11, 2017
@cherrypy cherrypy deleted a comment Oct 12, 2017
# Python normally converts this but not when the staticdir is
# supplied in long-path notation, eg: \\?\C:\static\js/script.js
if os.name == 'nt':
branch = branch.replace('/', '\\')

This comment has been minimized.

@webknjaz

webknjaz Oct 12, 2017
Member

Please use r'\'

This comment has been minimized.

@Safihre

Safihre Oct 12, 2017
Author Contributor

That's not possible:

E       branch = branch.replace('/', r'\')
E                                        ^
E   SyntaxError: EOL while scanning string literal

EDIT: not possible in general with only 1 char, not just as function parameter.

This comment has been minimized.

@webknjaz

webknjaz Oct 15, 2017
Member

Oh.. I see

@cherrypy cherrypy deleted a comment Oct 12, 2017
@webknjaz
Copy link
Member

@webknjaz webknjaz commented Oct 13, 2017

@Safihre plz also rebase your branch in order for it to pick up my workaround for codecov's bug

@cherrypy cherrypy deleted a comment Oct 13, 2017
@@ -185,6 +189,14 @@ def test_static(self):
# we just check the content
self.assertMatchesBody('^Dummy stylesheet')

@pytest.mark.skipif(platform.system() != 'Windows', reason='Windows only')
def test_static_longpath(self):
"""Test serving of a file in subdir of a Windows long-path staticdir"""

This comment has been minimized.

@webknjaz

webknjaz Oct 15, 2017
Member

Please add a period at the end of docstring. This is required by PEP 257.

This comment has been minimized.

@webknjaz

webknjaz Oct 15, 2017
Member

(this is the last nitpick and I'll merge PR after fixing it)

@cherrypy cherrypy deleted a comment Oct 15, 2017
@webknjaz webknjaz merged commit b2ceaae into cherrypy:master Oct 15, 2017
3 of 4 checks passed
3 of 4 checks passed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
codacy/pr Good work! A positive pull request.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
lgtm analysis: Python No alert changes
Details
@webknjaz
Copy link
Member

@webknjaz webknjaz commented Oct 15, 2017

@Safihre thank you!

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

Successfully merging this pull request may close these issues.

None yet

2 participants