zig icon indicating copy to clipboard operation
zig copied to clipboard

package: skip unpack errors on paths excluded by manifest

Open ianic opened this issue 2 years ago • 3 comments

The purpose of this is to prepare the ground for package manager related changes like: #17463 and #18089.

I found that there is are same logic implemented in both tar and git unpacking.

Tar pipeToFileSystem and git checkout are both: collecting file system errors, writing that errors to error bundle. Recursive directory copy and pipeToFileSystem are both creating Dir when writing file while Dir still doesn't exist.

Adding something new requires changes in a few places.

This collects file system logic in a single place and leaves tar/git with their primary job: decoding tarball or git pack.

I'll mark this draft hoping to get some kind of go/no go to implementing fixes to the above issues on top of this.

ianic avatar Mar 16 '24 20:03 ianic

I think both ways of doing it are reasonable. To me it is AAA BBB CCC vs ABC ABC ABC.

I think one or the other can be better depending on the problem, and in general beginner software developers have a bias towards the first one while intermediate programmers have a bias towards the second one.

For this part of the codebase, I did consider both options and reasoned that, while the former does involve having similar logic implemented multiple times, it makes up for it in the sense that you can more easily read the code for a particular fetch mechanism, and perhaps optimize it better.

I made this mistake in the backend of the compiler - I tried to do ABC ABC ABC with respect to CPU architecture, but it turned out to be untenable, and now it has been reworked to the AAA BBB CCC pattern.

It's difficult to tell objectively which way is better. I will say that I do have a preference for status quo.

andrewrk avatar Mar 17 '24 22:03 andrewrk

Fixes #18089 (checks the remaining 2 items).

Before this any unpack error will be raised after the unpack step. Now this is postponed until we get the manifest. Manifest is then used to filter errors for the files which are not included.

There are two types of unpack errors:

  • files with the same name but different casing, fails on non case sensitive file systems (Windows, macOS)
  • symlinks, creation fails on Windows if they are disabled

Here is example which has both:

sample
├── build.zig
├── build.zig.zon
├── dir1
│   └── dir2
│       └── dir3
│           ├── file1
│           ├── file2
│           └── file3
├── dir4
│   └── dir5
│       └── dir6
│           ├── file1 -> ../../../dir1/dir2/dir3/file1
│           └── file2_symlink -> ../../../dir1/dir2/dir3/file2
├── dir7
│   └── dir8
│       └── dir9
│           ├── file4
│           └── File4
└── src
    ├── main.zig
    └── root.zig

It is packed in sample.tar, and once more in sample_src.tar where build.zig.zon has only src in paths (excluded all problematic folders):

    .paths = .{
        "src",
    },

On Linux both tars get unpacked. On Windows and macOS both fail before this changes and the second succeeds after this change.

Windows (with disabled symlinks), before both failing:

C:\Users\ianic\code\zig-bin\zig.exe build --global-cache-dir zig-global-cache-release
C:\Users\ianic\code\issues\fetch\build.zig.zon:78:20: error: unable to unpack tarball
            .url = "http://10.211.55.26:8000/sample.tar",
                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
note: unable to create file 'dir7/dir8/dir9/file4': PathAlreadyExists
note: unable to create symlink from 'dir4/dir5/dir6/file1' to '../../../dir1/dir2/dir3/file1': AccessDenied
note: unable to create symlink from 'dir4/dir5/dir6/file2_symlink' to '../../../dir1/dir2/dir3/file2': AccessDenied
C:\Users\ianic\code\issues\fetch\build.zig.zon:85:20: error: unable to unpack tarball
            .url = "http://10.211.55.26:8000/sample_src.tar",
                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
note: unable to create file 'dir7/dir8/dir9/file4': PathAlreadyExists
note: unable to create symlink from 'dir4/dir5/dir6/file1' to '../../../dir1/dir2/dir3/file1': AccessDenied
note: unable to create symlink from 'dir4/dir5/dir6/file2_symlink' to '../../../dir1/dir2/dir3/file2': AccessDenied
PS C:\Users\ianic\code\issues\fetch> C:\Users\ianic\code\zig-bin\bin\zig.exe build --global-cache-dir zig-global-cache-release

after, just first is failing:

C:\Users\ianic\code\zig-io\build\stage3\bin\zig.exe build --global-cache-dir zig-global-cache-master
C:\Users\ianic\code\issues\fetch\build.zig.zon:78:20: error: unable to unpack
            .url = "http://10.211.55.26:8000/sample.tar",
                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
note: unable to create file 'dir7/dir8/dir9/file4': PathAlreadyExists
note: unable to create symlink from 'dir4/dir5/dir6/file1' to '../../../dir1/dir2/dir3/file1': AccessDenied
note: unable to create symlink from 'dir4/dir5/dir6/file2_symlink' to '../../../dir1/dir2/dir3/file2': AccessDenied

macOS, before both failing:

Fetch Packages [45/40] /Users/ianic/code/zig/issues/fetch/build.zig.zon:75:20: error: unable to unpack tarball
            .url = "file:///Users/ianic/code/zig/issues/fetch/sample.tar",
                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
note: unable to create file 'dir7/dir8/dir9/file4': PathAlreadyExists
/Users/ianic/code/zig/issues/fetch/build.zig.zon:80:20: error: unable to unpack tarball
            .url = "file:///Users/ianic/code/zig/issues/fetch/sample_src.tar",
                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

after, just first is failing:

Fetch Packages [41/40] /Users/ianic/code/zig/issues/fetch/build.zig.zon:75:20: error: unable to unpack
            .url = "file:///Users/ianic/code/zig/issues/fetch/sample.tar",
                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
note: unable to create file 'dir7/dir8/dir9/file4': PathAlreadyExists

I checked that fetch behavior is unchanged for a list of hopefully representative packages.

ianic avatar Mar 19 '24 21:03 ianic

Adding logic from another PR: #19111 which fixes #17779. Tar unpacking was removing the leading root folder. But there are packages without that leading folder that all fail with BadFileName. When one component is stripped from files in the root that leaves us with the empty file name. We are now packing without removing the root folder and then deciding whether to skip a single root folder.

This now fixes 0.12 milestone package manager related issues: #17779 and #18089. It also fixes #17460.

ianic avatar Mar 22 '24 20:03 ianic

Sorry to do this after you have put so much work into it.

No problem. Thanks for feedback. Will close this and try some other approach.

ianic avatar Mar 26 '24 21:03 ianic