Add support for non-unicode, bytes-only paths on Linux and *nix #121

pombredanne · 2017-12-20T17:04:27Z

This is a fix for #120
Signed-off-by: Philippe Ombredanne pombredanne@nexb.com

Signed-off-by: Philippe Ombredanne <pombredanne@nexb.com>

The approach is that unicode is used everywhere unless when on *nix and that real access to files is needed. In this case the patch is encoded to bytes using the filesystem encoding. Signed-off-by: Philippe Ombredanne <pombredanne@nexb.com>

Signed-off-by: Philippe Ombredanne <pombredanne@nexb.com>

coveralls · 2017-12-20T17:06:33Z

Coverage decreased (-0.9%) to 99.072% when pulling 7c7886f on pombredanne:unicode into ee3e335 on PyFilesystem:master.

willmcgugan · 2017-12-20T17:22:37Z

fs/base.py

@@ -599,14 +599,16 @@ def getsize(self, path):
        size = self.getdetails(path).size
        return size

-    def getsyspath(self, path):
+    def getsyspath(self, path, as_bytes=False):


I'm not overly keen on overloading getsyspath like this.

How about leaving getsyspath as returning unicode, and add a getnativepath which may return any format (presumably bytes for *nix).

@willmcgugan your call. I had started doing so but then I found it heavy: to have another method, another error, etc. And since your docstring for getsyspath states:

A system path is one recognized by the OS, that may be used outside of PyFilesystem (in an application or a shell for example). This method will get the corresponding system path that would be referenced by ``path``.

... I find it really hard to have another method with different docstring... since on *nix the real system path is bytes.

So what would happen if you were to get a syspath with as_bytes=False where the path couldn't be decoded as unicode?

as_bytes=False is the default so you still always get unicode out. With as_bytes=True you will always get bytes out using the filesystem encoding. There is no case I can think of where there would be any possible issue (unless if you have no filesystem encoding at all.... but that is the source of so many other possible errors and is a gross misconfiguration of the OS that's it is not worth catering to)

but again, your call if you think that would break some API. I think not since this is only additive with a default.

Will need to give this one some thought!

Note also that strictly speaking the conversion to bytes on *nix is only required on Python2. Python3 handles this fine internally (and the os.path and related stdlib have been sprinkled with plenty of os.fsencode/fsdecode as needed) .... So I could make the test for both *nix and py2 ... now the need to get bytes is still valid on Py2 and Py3 as a unicode fsdecoded path such as in #120 cannot be used in a *nix shell outside of Python unless it is fsencoded first.

The latest commit only deals with bytes internally on Python 2 and *nix. Everywhere else, this is unicode throughout. We still need of course the ability to fsencode some unicode, but frankly this could be left out entirely of the getsyspath method. Instead we could update the doc and stating that getsyspath always return Unicode and when and why this could be a problem, and how to fsencode the received unicode for use outside if needed.

This could be a happy middle ground.

@pombredanne Sounds good. But could you drop the as_bytes then? Calling this method with the new parameter will break with most of the other filesystems and all of the external filesystems.

I'm still favouring the getnativepath which may return bytes or unicode, but leaving getsyspath as returning (surrogate encoded) unicode. The default getnativepath could just return the same value as getsyspath

I'm still favouring the getnativepath which may return bytes or unicode, but leaving getsyspath as returning (surrogate encoded) unicode. The default getnativepath could just return the same value as getsyspath

Do you want this at all? I thing this is no longer needed as the doc explains that getsystempath is always unicode and that some use cases may need fsencoding to be usable

Signed-off-by: Philippe Ombredanne <pombredanne@nexb.com>

coveralls · 2017-12-20T17:26:09Z

Coverage decreased (-0.05%) to 99.902% when pulling 116c5d6 on pombredanne:unicode into ee3e335 on PyFilesystem:master.

pombredanne · 2017-12-20T17:50:42Z

@willmcgugan BTW, coveralls complains for bits that can be tested only when on macOS or Windows. #122 is needed to deal with this.... b ut then coverall is crass at merging reports from multiple runs last I checked, while codecode was OK there.

pombredanne · 2017-12-20T18:02:00Z

Sounds like this is decently passing the tests now and is ready for your review.

pombredanne · 2017-12-20T20:39:27Z

as a fun side note: with this patch you could then claim that you are the only path abstraction library that works on all OS and FS encodings on Py2 and Py3. There are none anywhere else. I searched hard.

Signed-off-by: Philippe Ombredanne <pombredanne@nexb.com>

coveralls · 2017-12-21T08:58:31Z

Coverage decreased (-0.02%) to 99.927% when pulling 257cc1f on pombredanne:unicode into ee3e335 on PyFilesystem:master.

Instead I added doc to explain that fsencode how can be used if needed. Signed-off-by: Philippe Ombredanne <pombredanne@nexb.com>

* Avoid code duplication with a new _get_validated_syspath() method * Remove as_bytes arg from getsyspath PyFilesystem#120 Signed-off-by: Philippe Ombredanne <pombredanne@nexb.com>

coveralls · 2017-12-21T09:23:06Z

Coverage decreased (-0.02%) to 99.926% when pulling 0ebc631 on pombredanne:unicode into ee3e335 on PyFilesystem:master.

pombredanne · 2017-12-21T09:40:31Z

@willmcgugan With the latest commits, there is no change in API needed at all, and the internal use of fsencoded bytes is limited to *nix and Py2.

willmcgugan

Thanks. Hope you don't mind my pickiness.

willmcgugan · 2017-12-21T09:56:28Z

setup.py

@@ -37,7 +37,8 @@
    install_requires=REQUIREMENTS,
    extras_require={
        "scandir :python_version < '3.5'": ['scandir~=1.5'],
-        ":python_version < '3.4'": ['enum34~=1.1.6']
+        ":python_version < '3.4'": ['enum34~=1.1.6'],
+        ":python_version < '3.0'": ['backports.os'],


I think the fsencode was introduced in Python3.2. Could you pin the backports.os module?

willmcgugan · 2017-12-21T09:58:17Z

requirements.txt

@@ -3,3 +3,4 @@ enum34==1.1.6 ; python_version < '3.4'
 pytz
 setuptools
 six==1.10.0
+backports.os


Could you pin this, and add the Python version syntax...

willmcgugan · 2017-12-21T10:05:02Z

fs/base.py

@@ -599,14 +599,16 @@ def getsize(self, path):
        size = self.getdetails(path).size
        return size

-    def getsyspath(self, path):
+    def getsyspath(self, path, as_bytes=False):


@pombredanne Sounds good. But could you drop the as_bytes then? Calling this method with the new parameter will break with most of the other filesystems and all of the external filesystems.

I'm still favouring the getnativepath which may return bytes or unicode, but leaving getsyspath as returning (surrogate encoded) unicode. The default getnativepath could just return the same value as getsyspath

willmcgugan · 2017-12-21T10:16:16Z

fs/osfs.py

        return names

    def makedir(self, path, permissions=None, recreate=False):
        self.check()
        mode = Permissions.get_mode(permissions)
+        path = path and fsdecode(path) or path


There's a convention we use that you don't modify a path variable once its passed in. The reason is that when you raise an error containing the path, the error message will contain the same path that the developer called the method with.

Could you replace this pattern with path = fsdecode(path) if path else path ? It's more familiar for developers who came to Python recently.

I can use a _path instead alright. But the and/or is more pythonic than if/else in my book. I can admit though that it may not be obvious ;)

* This was mistakenly left over * Remove as_bytes arg from getsyspath PyFilesystem#120 Signed-off-by: Philippe Ombredanne <pombredanne@nexb.com>

coveralls · 2017-12-21T10:31:56Z

Coverage decreased (-0.02%) to 99.926% when pulling a48d134 on pombredanne:unicode into ee3e335 on PyFilesystem:master.

Signed-off-by: Philippe Ombredanne <pombredanne@nexb.com>

coveralls · 2017-12-21T10:35:33Z

Coverage decreased (-0.02%) to 99.926% when pulling 3504187 on pombredanne:unicode into ee3e335 on PyFilesystem:master.

@willmcgugan

Following @willmcgugan in PyFilesystem#121 this is: - removing and/or shortcuts and - does not override path arg variables Signed-off-by: Philippe Ombredanne <pombredanne@nexb.com>

pombredanne · 2017-12-21T11:26:46Z

Thanks. Hope you don't mind my pickiness.

I can be much more picky than this ;)

coveralls · 2017-12-21T11:27:50Z

Coverage decreased (-0.8%) to 99.143% when pulling 77c0548 on pombredanne:unicode into ee3e335 on PyFilesystem:master.

pombredanne · 2017-12-21T11:28:24Z

@willmcgugan The latest should have all your comments/feedback applied.

pombredanne · 2017-12-21T16:35:41Z

@willmcgugan Some thing went south on the Py3 tests. Let me review this

* I had somehow introduced a regression with the previous commit Signed-off-by: Philippe Ombredanne <pombredanne@nexb.com>

coveralls · 2017-12-22T09:55:41Z

Coverage increased (+0.0002%) to 99.951% when pulling b7d9e87 on pombredanne:unicode into ee3e335 on PyFilesystem:master.

pombredanne · 2017-12-22T09:58:38Z

I cleaned up the regression I had introduced. We should be all good now.

pombredanne · 2017-12-22T09:59:02Z

Unless you have some strong objections left about this PR, I will build a local wheel until you have made a release and start using this in ScanCode. A pypi release soon enough would be welcomed!

pombredanne · 2017-12-22T16:10:38Z

Hold on merging this. There are plenty of issues left to test and fix on *nix with non-decodable paths. I am playing with beautiful crashes so far.

willmcgugan · 2017-12-22T16:22:50Z

@pombredanne I think I can live without the getnativepath for now. I'll consider adding it myself later.

No rush, will be AFK for a few days over xmas.

willmcgugan · 2018-01-07T15:14:26Z

@pombredanne Did you resolve the crashes? Is it ready for review?

willmcgugan · 2018-05-02T16:03:13Z

The code has moved on a bit. And I've attempted to tackle this issue in #167

willmcgugan · 2018-05-12T12:02:43Z

Closing in favour of #167

Thanks for taking the lead on this one @pombredanne

pombredanne added 5 commits December 20, 2017 12:37

Use @pjdelport backports.os on Py2 PyFilesystem#120

810ee9b

Signed-off-by: Philippe Ombredanne <pombredanne@nexb.com>

Add new tests for non-unicode bytes paths PyFilesystem#120

b3c22c7

Signed-off-by: Philippe Ombredanne <pombredanne@nexb.com>

Add optional "as_bytes" arg PyFilesystem#120

ce6ae08

Signed-off-by: Philippe Ombredanne <pombredanne@nexb.com>

Use fsencoded paths on *nix PyFilesystem#120

8f13427

The approach is that unicode is used everywhere unless when on *nix and that real access to files is needed. In this case the patch is encoded to bytes using the filesystem encoding. Signed-off-by: Philippe Ombredanne <pombredanne@nexb.com>

Ignore tmp dir

7c7886f

Signed-off-by: Philippe Ombredanne <pombredanne@nexb.com>

pombredanne changed the title ~~[WPI] Add support for non-unicode, bytes-only paths on Linux and *nix~~ [WIP] Add support for non-unicode, bytes-only paths on Linux and *nix Dec 20, 2017

willmcgugan reviewed Dec 20, 2017

View reviewed changes

Ensure tests pass on Python3 PyFilesystem#120

116c5d6

Signed-off-by: Philippe Ombredanne <pombredanne@nexb.com>

pombredanne mentioned this pull request Dec 20, 2017

fs.listdir and UnicodeError #120

Closed

pombredanne changed the title ~~[WIP] Add support for non-unicode, bytes-only paths on Linux and *nix~~ Add support for non-unicode, bytes-only paths on Linux and *nix Dec 20, 2017

pombredanne added 2 commits December 21, 2017 09:55

Remove trailing line return

9f3626d

Signed-off-by: Philippe Ombredanne <pombredanne@nexb.com>

fsencode only on *nix and Python2 PyFilesystem#120

257cc1f

Signed-off-by: Philippe Ombredanne <pombredanne@nexb.com>

pombredanne added 2 commits December 21, 2017 10:18

Remove as_bytes arg from getsyspath PyFilesystem#120

6d5181b

Instead I added doc to explain that fsencode how can be used if needed. Signed-off-by: Philippe Ombredanne <pombredanne@nexb.com>

Refactor common checks in _get_validated_syspath() PyFilesystem#120

0ebc631

* Avoid code duplication with a new _get_validated_syspath() method * Remove as_bytes arg from getsyspath PyFilesystem#120 Signed-off-by: Philippe Ombredanne <pombredanne@nexb.com>

pombredanne mentioned this pull request Dec 21, 2017

Allow a plugin to provide more than one option nexB/scancode-toolkit#787

Closed

willmcgugan requested changes Dec 21, 2017

View reviewed changes

Remove as_bytes arg PyFilesystem#120

a48d134

* This was mistakenly left over * Remove as_bytes arg from getsyspath PyFilesystem#120 Signed-off-by: Philippe Ombredanne <pombredanne@nexb.com>

Pin backports.os with correct Python version PyFilesystem#120

3504187

Signed-off-by: Philippe Ombredanne <pombredanne@nexb.com>

Improve Python idioms and variable overriding PyFilesystem#120

77c0548

Following @willmcgugan in PyFilesystem#121 this is: - removing and/or shortcuts and - does not override path arg variables Signed-off-by: Philippe Ombredanne <pombredanne@nexb.com>

Cleanup scandir bytes vs unicode handling PyFilesystem#120

b7d9e87

* I had somehow introduced a regression with the previous commit Signed-off-by: Philippe Ombredanne <pombredanne@nexb.com>

willmcgugan closed this May 12, 2018

Add support for non-unicode, bytes-only paths on Linux and *nix #121

Add support for non-unicode, bytes-only paths on Linux and *nix #121

Conversation

pombredanne commented Dec 20, 2017

coveralls commented Dec 20, 2017

Choose a reason for hiding this comment

pombredanne Dec 20, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pombredanne Dec 20, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Dec 20, 2017 • edited

pombredanne commented Dec 20, 2017 • edited

pombredanne commented Dec 20, 2017

pombredanne commented Dec 20, 2017

coveralls commented Dec 21, 2017 • edited

coveralls commented Dec 21, 2017 • edited

pombredanne commented Dec 21, 2017

willmcgugan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Dec 21, 2017 • edited

coveralls commented Dec 21, 2017 • edited

pombredanne commented Dec 21, 2017

coveralls commented Dec 21, 2017

pombredanne commented Dec 21, 2017

pombredanne commented Dec 21, 2017

coveralls commented Dec 22, 2017 • edited

pombredanne commented Dec 22, 2017

pombredanne commented Dec 22, 2017 • edited

pombredanne commented Dec 22, 2017

willmcgugan commented Dec 22, 2017

willmcgugan commented Jan 7, 2018

willmcgugan commented May 2, 2018

willmcgugan commented May 12, 2018 • edited

pombredanne Dec 20, 2017 •

edited

pombredanne Dec 20, 2017 •

edited

coveralls commented Dec 20, 2017 •

edited

pombredanne commented Dec 20, 2017 •

edited

coveralls commented Dec 21, 2017 •

edited

coveralls commented Dec 21, 2017 •

edited

coveralls commented Dec 21, 2017 •

edited

coveralls commented Dec 21, 2017 •

edited

coveralls commented Dec 22, 2017 •

edited

pombredanne commented Dec 22, 2017 •

edited

willmcgugan commented May 12, 2018 •

edited