Page MenuHome

Don't resolve symlinks on packed paths
ClosedPublic

Authored by Sybren A. Stüvel (sybren) on Jan 26 2020, 10:34 PM.

Details

Summary

Previously, path.resolve was used to resolve paths.
This had 3 effects:

  1. It made the path absolute
  2. Normalized the path
  3. Resolve symlinks and network drives

Because no. 3 was undesirable (see D6500), path.resolve was replaced
with os.path.abspath which just does the first two.

Diff Detail

Repository
rBAST Blender Asset Tracer
Branch
resolve-paths (branched from master)
Build Status
Buildable 6389
Build 6389: arc lint + arc unit

Event Timeline

  • Make resolve_path backwards compatible with 3.5
Sybren A. Stüvel (sybren) requested changes to this revision.EditedMon, Mar 2, 11:08 AM

Please don't add files with names like like utils, it doesn't mean anything. The resolve_path function can be placed in bpathlib.py.

I think it's a good idea to add one or more unittests that test the resolve_path() function (the test you added is good, but it only indirectly tests the behaviour). Most functionality can be tested for easily. It's just the NOT resolving of mapped drive letters to UNC notation that I wouldn't know how to test. Maybe that's possible with some intrusive use of unittest.mock?

As for the function name, I'm not convinced resolve_path is the right name, as it shouldn't resolve symlinks at all. Maybe canonical is a better name, as it in a way returns the canonical path.

blender_asset_tracer/pack/__init__.py
239

bfile_path is already the result of utils.resolve_path(), so it doesn't have to be resolved again.

371

This line is getting quite long. I think it's cleaner to do this instead:

path = utils.resolve_path(path)
project_path = utils.resolve_path(self.project)

try:
    path.relative_to(self.project)
except ValueError:
    return False
return True

Also please don't remove my comment that explains why this is happening at all.

blender_asset_tracer/pack/utils.py
6

This really needs an explanation as to why it's different than path.resolve(). It should include what it does the same (resolving ../../), and what it does differently (NOT resolving symlinks or mapped drive letters).

This revision now requires changes to proceed.Mon, Mar 2, 11:08 AM

I'm taking over this patch to get it into BAT.

This revision now requires review to proceed.Tue, Mar 17, 10:59 AM
  • Cleanup: from pathlib import Path, PosixPath
  • Moved utils.py to bpathlib.py, renamed function to make_absolute(), and added unit test
  • Rewrote symlinking unittest to be a bit more explicit on what is expected behaviour, and to test the actual files on the filesystem (not just library references in one file).
This revision was not accepted when it landed; it landed in state Needs Review.Tue, Mar 17, 4:05 PM
This revision was automatically updated to reflect the committed changes.

Thanks for the patch @Isaac Weaver (wisaac) . I took it a bit further and committed it as rBASTe4bf2e8e35a8: Improved path handling.