Page MenuHome

Resolve all packed paths
AbandonedPublic

Authored by Isaac Weaver (wisaac) on Mon, Dec 30, 11:04 PM.

Details

Summary

Issue:
If the blender file path passed to pack.Packer is a symlink, BAT will raise an AssertianError:

    468             # exist on the local filesystem.
    469             bfile_pp = action.new_path
--> 470             assert bfile_pp is not None
    471 
    472             # Use tempfile to create a unique name in our temporary directoy.

AssertionError:

Platform: Mac OS

Steps to reproduce:

  1. Create a blender project with a linked library
  2. Symlink that project to another location
  3. Run the following: Packer(blendfile, blendfile.parent, output)

If blendfile is a Path pointing to a symlink, it will cause the above error.

Cause

The issue seems to be caused in strategise and _group_rewrites. An action is added in stretegise for the root .blend file (line 244) that's tied to the blendfile Path that's passed in (the unresolved path). In _group_rewrites, the action is updated for all its rewrites (line 369). However, it attempts to find the action using the resolved path, so it's unable to find the original action created for the root .blend file (instead, it creates a new action that doesn't have the new_path field set, which causes the AssertionError).

Fix:

This patch fixes the error by resolving blendfile so it doesn't create duplicate keys.

Diff Detail

Event Timeline

Thanks for the patch. Please describe it in the proper way, though. Currently there is no info about the OS (probably Linux, but that's just a guess), and no description of the error.

Having a unit test that tests this situation would also be a good idea, because this is a situation that could easily be overlooked during later development.

What is the effect of calling .resolve() here when running on Windows when the blend file is on a network-mounted drive? Say you have \\NAS\share mounted as S:, Path("S:/file.blend").resolve() will result in \\NAS\share\file.blend, which may interfere with other functionality.

blender_asset_tracer/pack/__init__.py
233

Path.resolve() already returns an absolute path, so calling .absolute() is unnecessary.

239

Calling bfile_path.resolve() here is now also redundant.

Isaac Weaver (wisaac) edited the summary of this revision. (Show Details)Tue, Dec 31, 3:21 PM
Isaac Weaver (wisaac) edited the summary of this revision. (Show Details)Tue, Dec 31, 3:24 PM
  • Remove redundant path resolve/absolute calls
Isaac Weaver (wisaac) marked 2 inline comments as done.
  • Add unit test for resolving symlinks

I updated the description and added a test. That's a good question about Windows. Unfortunately, I'm unable to test it at the moment. It already uses the resolved path for all the other assets, so if it would be an issue, it would probably already have problems without this patch.

tests/test_pack.py
4

Please keep the import statements ordered alphabetically. I see that I didn't either (tempfile should be imported before typing) but let's not add to my mess ;-)

201

This test is a good start. I do feel it should also check for existence of the other paths, though. I mean, doubly_linked_up.blend should pull other files into the BAT pack, and those should be checked for as well. It's nice that it doesn't cause an exception, but I'm not willing to trust that that means a perfect BAT pack ;-)

  • Test that all paths are rewritten in symlink test
Isaac Weaver (wisaac) marked an inline comment as done.
  • Alphabetize imports in packing test file
Sybren A. Stüvel (sybren) requested changes to this revision.Fri, Jan 3, 4:28 PM
Sybren A. Stüvel (sybren) added inline comments.
tests/test_pack.py
6

pathlib is already imported, so just use pathlib.Path instead.

195

This will fail on Windows due to permission errors (see the warning in the docs). I think it's best to just skip the test on that platform, with code like this at the start of the function:

if platform.system() == 'Windows':
    self.skipTest("Symlinks on Windows require Administrator rights")
This revision now requires changes to proceed.Fri, Jan 3, 4:28 PM
Isaac Weaver (wisaac) marked an inline comment as done.
  • Don't reimport Path
  • Skip the symlink path test on Windows
This revision is now accepted and ready to land.Mon, Jan 6, 11:22 AM
Sybren A. Stüvel (sybren) requested changes to this revision.EditedMon, Jan 6, 11:44 AM

I'm undoing my earlier acceptance, as I'm not convinced this patch (and more importantly my own code) is doing the right thing. Sorry for thinking about this in a broader sense this late in the review process.

Using relative paths makes things ambiguous, as you also have to know what path they are relative to; making paths absolute resolves this issue. Path.resolve() is the defacto way to make a pathlib.Path absolute. You can also see this in the Python documentation, where os.path.abspath() is translated to Path.resolve(). This has nasty side-effects, though, as it does three things at once:

  1. it makes the path absolute,
  2. it removes ../ components from the path,
  3. it resolves symlinks on macOS/Linux, and on Windows it rewrites mapped network drive letters to UNC notation (so S:\ to \\NAS\Shared for example).

The first two are desired, and are the reason why Path.resolve() is called in the first place. However, the third causes problems where symlinks are used to construct a project directory. This happens, for example, in the case of a Shaman checkout like this:

sybren@ws-sybren /render/_flamenco/shaman-checkout/66/5c792a921c534903999fd666 
% tree
.
├── lib
│   ├── char
│   │   ├── alpha
│   │   │   └── alpha.blend -> /render/_flamenco/shaman-file-store/stored/09/f71fc94b0effafc1688f5d823aa0303723425cfdb44f2179d510691e758ab6/1099549252.blob
│   │   ├── autumn
│   │   │   └── autumn.blend -> /render/_flamenco/shaman-file-store/stored/56/da45640a73fc86ccb81269b0da5bdba914584c920631f915c65fb94f0120b5/143113000.blob
│   │   ├── beta
│   │   │   ├── beta1.blend -> /render/_flamenco/shaman-file-store/stored/3a/1f8a282a6ed4aebeb37858e5422e2822daa5b046d89e4b439ec0e118ef2bbd/1385702128.blob
│   │   │   ├── beta2.blend -> /render/_flamenco/shaman-file-store/stored/6a/0793fe002e7e38890c45f71eac3eb88c27286f970c434f9fc2306836e23af1/1385699008.blob
│   │   │   ├── beta3.blend -> /render/_flamenco/shaman-file-store/stored/f3/3b005ae432b7e7dc3ad46c3730e7420799ea879ade8d052ee51080adbe774e/1385686744.blob
│   │   │   ├── beta4.blend -> /render/_flamenco/shaman-file-store/stored/9e/d5b48027b5e36cd969e0c2e014ddf7ba1ddeb03bf1d2eb62ec0601e51c4f6f/1385706072.blob
│   │   │   └── beta5.blend -> /render/_flamenco/shaman-file-store/stored/fe/4c216db8d06582ed830a22686a3aa0ca82ddd8b4320a4ad5c383f59743087d/1385707912.blob
│   │   └── spring
│   │       ├── maps
│   │       │   ├── TEX_spring_boots_symbol.png -> /render/_flamenco/shaman-file-store/stored/e7/16af5a43be2fda938b01ef5dcb70d3b61cdb4de1298c7e88fd0d4c646970ec/620678.blob
│   │       │   ├── TEX_spring_eyes_color.png -> /render/_flamenco/shaman-file-store/stored/ea/3f0784fe4a516126d31df4cdac3bb9681f2e475b4a9f40a79a03a3db78dc3a/2250175.blob
│   │       │   ├── TEX_spring_hairband_worn.png -> /render/_flamenco/shaman-file-store/stored/8c/095e1f1f608a2de22b466503623838732000686d7d19486ef9e3d8121bbd41/59791.blob
│   │       │   ├── TEX_spring_hair_transparency.png -> /render/_flamenco/shaman-file-store/stored/87/de2cca6f2e87f75d270a0864fa1db275dd6b701facfee13d4cca9598d7aa71/53181.blob
│   │       │   ├── TEX_spring_hands_bump.png -> /render/_flamenco/shaman-file-store/stored/4b/b0111d9eae66974fb8b097e0f60a5dcabe323fbe6434992e06fb492845e362/746769.blob
│   │       │   ├── TEX_spring_hands_color.png -> /render/_flamenco/shaman-file-store/stored/39/325f766831937aaa88960748e0517191032d196cde6ee4cce0bcef624de462/3928798.blob
│   │       │   ├── TEX_spring_hands_dirt.png -> /render/_flamenco/shaman-file-store/stored/99/4a265e4fac601b3f2e605b8a99f7d683cb8c72cdda2680f3ef61d65320663c/442573.blob
│   │       │   ├── TEX_spring_hands_spec.png -> /render/_flamenco/shaman-file-store/stored/d5/e9f4e6e43f3ac928e6cd230136a833c2263c75f6f98c997fbf055aacdceaec/2083809.blob
│   │       │   ├── TEX_spring_head_bump.png -> /render/_flamenco/shaman-file-store/stored/39/4d890dbe44a2f51eef47236364a23bf51fdbafd62da777974bfc7e349e1a7f/441297.blob
│   │       │   ├── TEX_spring_head_color.png -> /render/_flamenco/shaman-file-store/stored/b9/7d04fbf3390b92a6a4dcd0565e4323738eb253d425452735c62f412154618d/5188762.blob
│   │       │   ├── TEX_spring_head_spec.png -> /render/_flamenco/shaman-file-store/stored/86/0b8f225cc386e41430dffeb1bc1488e02cd4c92a4a08295fd38310862b48e5/1193927.blob
│   │       │   ├── TEX_spring_jacket_color.png -> /render/_flamenco/shaman-file-store/stored/99/9e57144d4a754fe2341cf71a7a34013edb6283634890c91a6b17cd685591ce/4494990.blob
│   │       │   ├── TEX_spring_pants_disp.png -> /render/_flamenco/shaman-file-store/stored/04/898627e988f86537252a87d4ef0a0b51b0e3056fe9f7a0c20a71a8643eb3c8/10486242.blob
│   │       │   ├── TEX_spring_pants_worn.png -> /render/_flamenco/shaman-file-store/stored/0c/9f829e3066a2b749b5ce03b963688ba78acf68c0a1a038e7275f17106d351c/225311.blob
│   │       │   ├── TEX_spring_pullover_disp.png -> /render/_flamenco/shaman-file-store/stored/9a/ce36f26283cafb464474742d4858276897e699b9d8f315aa6c1a31df0a7b94/8844413.blob
│   │       │   └── TEX_spring_scarf_worn.png -> /render/_flamenco/shaman-file-store/stored/55/b58852f40576c3b6fffbc09f79818e5f7ac89ac631addc30dd4d9ff13ca95e/133385.blob
...

The BAT pack should result in the symlinked files in the way they are symlinked, and not using the naming of the targets of the symlinks. In the above example, I would expect lib/char/alpha/alpha.blend to be in the BAT pack, and not /render/_flamenco/shaman-file-store/stored/09/f71fc94b0effafc1688f5d823aa0303723425cfdb44f2179d510691e758ab6/1099549252.blob.

I think that to solve this properly all calls to Path.resolve() should be removed, and replaced by a call to a new function that only performs the two desired functions of Path.resolve(). @Isaac Weaver (wisaac) would you be up for doing this?

This revision now requires changes to proceed.Mon, Jan 6, 11:44 AM
Isaac Weaver (wisaac) marked 2 inline comments as done.Tue, Jan 7, 4:08 AM

Ok, makes sense. I'll try to get a patch for that in the next few days.

I finally had time to work on this. See D6676