package manager: handle archives without leading root folder #19111

ianic · 2024-02-27T19:00:32Z

Instead of striping leading root folder while unpacking tar, tar archive is unpacked as is into temporary directory. Then we check result and decide whether to skip root folder. That allows us to handle archives which don't have that leading root folder.

This if follow up of PR #19090.

Fixes #17779

src/Package/Fetch.zig

andrewrk

This does the behavior for any kind of resource, however, this logic is intended to only be executed for archives.

andrewrk · 2024-03-26T00:38:59Z

src/Package/Fetch.zig

    const rand_int = std.crypto.random.int(u64);
-    const tmp_dir_sub_path = "tmp" ++ s ++ Manifest.hex64(rand_int);
+    const tmp_dir_sub_path = "tmp" ++ s ++ Manifest.hex64(rand_int); // root of the temporary directory
+    var tmp_package_root_sub_path: ?[]const u8 = null; // package root inside temporary directory


With some code restructuring, this does not need to be a variable. Notice how the surrounding code avoids using variables, with the exception of loops and resource management.

andrewrk · 2024-03-26T00:39:42Z

src/Package/Fetch.zig

    const cache_root = f.job_queue.global_cache;
    const rand_int = std.crypto.random.int(u64);
-    const tmp_dir_sub_path = "tmp" ++ s ++ Manifest.hex64(rand_int);
+    const tmp_dir_sub_path = "tmp" ++ s ++ Manifest.hex64(rand_int); // root of the temporary directory


The comment disagrees with the local's name. If you pick a good name instead then there is no need to comment about the meaning of the name.

andrewrk

Thank you. I think this is the right approach.

I identified 3 syscall optimizations to be implemented before merging. I believe these changes will result in equivalent - if not more - maintainability for this code, while being more optimal.

andrewrk · 2024-04-02T21:20:42Z

src/Package/Fetch.zig

+fn archivePackageDir(arena: Allocator, out_dir: fs.Dir) !?[]const u8 {
+    var iter = out_dir.iterate();
+    if (try iter.next()) |entry| {
+        if (try iter.next() == null and entry.kind == .directory) {
+            return try arena.dupe(u8, entry.name);
+        }
+    }
+    return null;
+}


This information should be instead tracked by pipeToFileSystem to avoid these two syscalls (open and readdir).

andrewrk · 2024-04-02T21:32:48Z

src/Package/Fetch.zig

    uri_path: []const u8,
    tmp_directory: Cache.Directory,
-) RunError!void {
+) RunError!?[]const u8 {


👍 Enhancing this function to support returning a sub-path inside the temporary directory makes sense because otherwise the tar unpacking code would need to sometimes move all the top-level entries.

It took me a while to understand the meaning of this return value. Let's make it easier for the next person by adding a doc comment to the function to explain what return value means.

Something like this:

/// A `null` return value indicates the `tmp_directory` is populated directly with the package contents. /// A non-null return value means that the package contents are inside a sub-directory indicated by the named path.

andrewrk · 2024-04-02T21:39:45Z

src/Package/Fetch.zig

+        if (package_dir) |dir_name| {
+            // Position tmp_directory to dir_name inside tmp_dir_sub_path.
+            const path = try cache_root.join(arena, &.{ tmp_dir_sub_path, dir_name });
+            const handle = tmp_directory.handle.openDir(dir_name, .{ .iterate = true }) catch |err| {


No need to open the sub directory since the parent is already open. The Path abstraction already has a sub_path field. It looks like you may need to update computeHash to accept Path rather than Directory.

andrewrk · 2024-04-02T21:44:48Z

src/Package/Fetch.zig

+    // Remove temporary directory root.
+    cache_root.handle.deleteTree(tmp_dir_sub_path) catch {};


Syscall should be avoided when it is known to not exist already.

When it is needed, it is known to be an empty directory, so it is better to use deleteDir.

Prior to [this](ziglang@ef9966c#diff-08c935ef8c633bb630641d44230597f1cff5afb0e736d451e2ba5569fa53d915R805) commit tar was not a valid extension. After that this one is valid case.

Unpack tar without removing leading root folder. Then find package root in unpacked tmp folder.

From review comments: ziglang#19111

While iterating over all files in tarball set root_dir in diagnostic if there is single root in tarball. Will be used in package manager with strip_components = 0 to find the root of the fetched package.

Based on comment: ziglang#19111 (comment) computeHash finds all files in temporary directory. There is no difference on what path are they. When calculating hash normalized_path must be set relative to package root. That's the place where we strip root if needed.

Based on: ziglang#19111 (comment) In pipeToFileSystem we are already iterating over all files in tarball. Inspecting them there for existence of single root folder saves two syscalls later.

andrewrk

Nice work, thank you.

Filter should be applied on path where package root folder (if there is any) is stripped. Manifest is inside package root and has paths relative to package root not temporary directory root.

ianic · 2024-04-04T00:17:40Z

I made a bug in handling paths while filtering files not included in manifest!

Those paths are relative to the package root not to the temporary directory root. After removing reopening temporary directory to the package root those two are different in computeHash. There is entry file system path and entry package path. First has to be used for file system operations (deleting, reading), package path has to be used for filtering and hashing.

Fixed now.

Prepare test cases, store them in Fetch/testdata. They cover changes in this PR as well from previous one ziglang#19111.

From review comments: ziglang#19111

Based on comment: ziglang#19111 (comment) computeHash finds all files in temporary directory. There is no difference on what path are they. When calculating hash normalized_path must be set relative to package root. That's the place where we strip root if needed.

Based on: ziglang#19111 (comment) In pipeToFileSystem we are already iterating over all files in tarball. Inspecting them there for existence of single root folder saves two syscalls later.

Prepare test cases, store them in Fetch/testdata. They cover changes in this PR as well from previous one ziglang#19111.

ianic mentioned this pull request Feb 27, 2024

package: allow setting tar_strip_components #19090

Closed

andrewrk self-requested a review February 28, 2024 20:43

andy5995 reviewed Mar 10, 2024

View reviewed changes

src/Package/Fetch.zig Outdated Show resolved Hide resolved

ianic force-pushed the no_strip_components branch from c389f67 to 716adc0 Compare March 20, 2024 21:21

ianic mentioned this pull request Mar 22, 2024

package: skip unpack errors on paths excluded by manifest #19324

Closed

andrewrk requested changes Mar 26, 2024

View reviewed changes

ianic requested a review from andrewrk April 2, 2024 17:09

andrewrk requested changes Apr 2, 2024

View reviewed changes

ianic added 9 commits April 3, 2024 17:06

fetch: fix failing test

bc50767

Prior to [this](ziglang@ef9966c#diff-08c935ef8c633bb630641d44230597f1cff5afb0e736d451e2ba5569fa53d915R805) commit tar was not a valid extension. After that this one is valid case.

package manager: don't strip components in tar

a5a928b

Unpack tar without removing leading root folder. Then find package root in unpacked tmp folder.

fix typo

15ca0c9

fetch: find package root only for archives

c4261bf

fetch: save syscall, and add comment

24304a4

From review comments: ziglang#19111

tar: find package root dir in pipeToFileSystem

b1e70ed

While iterating over all files in tarball set root_dir in diagnostic if there is single root in tarball. Will be used in package manager with strip_components = 0 to find the root of the fetched package.

fetch: update comments

363acf4

fetch: use package root detection pipeToFileSystem

8545cb0

Based on: ziglang#19111 (comment) In pipeToFileSystem we are already iterating over all files in tarball. Inspecting them there for existence of single root folder saves two syscalls later.

ianic force-pushed the no_strip_components branch from ffb0460 to 8545cb0 Compare April 3, 2024 19:25

ianic requested a review from andrewrk April 3, 2024 19:26

andrewrk approved these changes Apr 3, 2024

View reviewed changes

andrewrk enabled auto-merge April 3, 2024 20:22

ianic disabled auto-merge April 3, 2024 23:18

ianic marked this pull request as draft April 3, 2024 23:18

fetch: fix manifest included paths filtering

a60b7af

Filter should be applied on path where package root folder (if there is any) is stripped. Manifest is inside package root and has paths relative to package root not temporary directory root.

ianic marked this pull request as ready for review April 4, 2024 00:08

andrewrk enabled auto-merge April 4, 2024 00:18

andrewrk merged commit e5d9002 into ziglang:master Apr 4, 2024

ianic deleted the no_strip_components branch April 4, 2024 14:27

ianic added a commit to ianic/zig that referenced this pull request Apr 4, 2024

fetch: save test cases

3efac94

Prepare test cases, store them in Fetch/testdata. They cover changes in this PR as well from previous one ziglang#19111.

ianic mentioned this pull request Apr 4, 2024

package manager: filter unpack errors on paths excluded by manifest #19500

Merged

RossComputerGuy pushed a commit to MidstallSoftware/zig that referenced this pull request Apr 5, 2024

fetch: save syscall, and add comment

9b7f728

From review comments: ziglang#19111

ianprime0509 mentioned this pull request Apr 6, 2024

build: unexpected panic "unable to find module" #19561

Closed

ianic added a commit to ianic/zig that referenced this pull request Apr 6, 2024

fetch: save test cases

13fc284

Prepare test cases, store them in Fetch/testdata. They cover changes in this PR as well from previous one ziglang#19111.

ianic added a commit to ianic/zig that referenced this pull request Apr 9, 2024

fetch: save test cases

b422e4a

Prepare test cases, store them in Fetch/testdata. They cover changes in this PR as well from previous one ziglang#19111.

zxubian mentioned this pull request May 6, 2024

zig package manager zig-gamedev/zig-gamedev#327

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

package manager: handle archives without leading root folder #19111

package manager: handle archives without leading root folder #19111

Uh oh!

ianic commented Feb 27, 2024

Uh oh!

Uh oh!

andrewrk left a comment

Uh oh!

andrewrk Mar 26, 2024

Uh oh!

andrewrk Mar 26, 2024

Uh oh!

andrewrk left a comment

Uh oh!

andrewrk Apr 2, 2024

Uh oh!

andrewrk Apr 2, 2024

Uh oh!

andrewrk Apr 2, 2024

Uh oh!

andrewrk Apr 2, 2024

Uh oh!

andrewrk left a comment

Uh oh!

ianic commented Apr 4, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		// Remove temporary directory root.
		cache_root.handle.deleteTree(tmp_dir_sub_path) catch {};

Uh oh!

package manager: handle archives without leading root folder #19111

package manager: handle archives without leading root folder #19111

Uh oh!

Conversation

ianic commented Feb 27, 2024

Uh oh!

Uh oh!

andrewrk left a comment

Choose a reason for hiding this comment

Uh oh!

andrewrk Mar 26, 2024

Choose a reason for hiding this comment

Uh oh!

andrewrk Mar 26, 2024

Choose a reason for hiding this comment

Uh oh!

andrewrk left a comment

Choose a reason for hiding this comment

Uh oh!

andrewrk Apr 2, 2024

Choose a reason for hiding this comment

Uh oh!

andrewrk Apr 2, 2024

Choose a reason for hiding this comment

Uh oh!

andrewrk Apr 2, 2024

Choose a reason for hiding this comment

Uh oh!

andrewrk Apr 2, 2024

Choose a reason for hiding this comment

Uh oh!

andrewrk left a comment

Choose a reason for hiding this comment

Uh oh!

ianic commented Apr 4, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants