zig icon indicating copy to clipboard operation
zig copied to clipboard

build: use absolute paths to local and global cache dirs

Open Jan200101 opened this issue 1 year ago • 1 comments

This prevents issues where the current working directory is changed and relative path becomes invalid.

I've created a small project to reproduce this issue https://github.com/Jan200101/zig-bug-local-cache-dep simply compare the outputs of zig build and zig build --global-cache-dir zig-gcache

With 0.12: (this was also tested against master and results in the same behavior)

$ /usr/bin/zig build --global-cache-dir zig-gcache
freetype_dep.path: zig-gcache/p/1220b81f6ecfb3fd222f76cf9106fecfa6554ab07ec7fdc4124b9bb063ae2adf969d

With this PR:

$ zig build --global-cache-dir zig-gcache
freetype_dep.path: /tmp/tmp.DRt3KNH1Yc/zig-gcache/p/1220b81f6ecfb3fd222f76cf9106fecfa6554ab07ec7fdc4124b9bb063ae2adf969d

I don't know if zig fetch or other commands could have the same issue, thus far I only ran into it with zig build.

Jan200101 avatar May 26 '24 15:05 Jan200101

IIRC, unfortunately, realpath is banned in new code inside compiler. As an alternative, you can instead convert it to absolute path based on cwd path, like in introspect.resolvePath. Something like this:

introspect: unify logic for finding global cache dir and
make `resolveGlobalCacheDir` always return absolute path.

(Change summary above and message below as you like)

If global cache dir is relative, `build_root` decl for packages in "dependencies.zig"
will also be relative. Since `getPath2` resolves `dep.sub_path` based on `dep.builder.build_root`,
it will also return relative path, which contradicts doc comment:
"""
// Inside LazyPath.getPath2
.dependency => |dep| return dep.dependency.builder.pathFromRoot(dep.sub_path),

// Inside pathFromRoot
return b.pathResolve(&.{ b.build_root.path orelse ".", sub_path });
"

After this commit, path to global cache (and `build_root` because of this) is always absolute.

diff --git a/src/introspect.zig b/src/introspect.zig
index 341b1cddeb..cac8f351d7 100644
--- a/src/introspect.zig
+++ b/src/introspect.zig
@@ -6,6 +6,8 @@ const fs = std.fs;
 const Compilation = @import("Compilation.zig");
 const build_options = @import("build_options");
 
+const getWasiPreopen = @import("main.zig").getWasiPreopen;
+
 /// Returns the sub_path that worked, or `null` if none did.
 /// The path of the returned Directory is relative to `base`.
 /// The handle of the returned Directory is open.
@@ -79,24 +81,63 @@ pub fn findZigLibDirFromSelfExe(
     return error.FileNotFound;
 }
 
-/// Caller owns returned memory.
-pub fn resolveGlobalCacheDir(allocator: mem.Allocator) ![]u8 {
-    if (builtin.os.tag == .wasi)
-        @compileError("on WASI the global cache dir must be resolved with preopens");
-
-    if (try std.zig.EnvVar.ZIG_GLOBAL_CACHE_DIR.get(allocator)) |value| return value;
-
-    const appname = "zig";
-
-    if (builtin.os.tag != .windows) {
-        if (std.zig.EnvVar.XDG_CACHE_HOME.getPosix()) |cache_root| {
-            return fs.path.join(allocator, &[_][]const u8{ cache_root, appname });
-        } else if (std.zig.EnvVar.HOME.getPosix()) |home| {
-            return fs.path.join(allocator, &[_][]const u8{ home, ".cache", appname });
-        }
+/// Returns global cache directory.
+/// `path` field is an absolute path in all cases except WASI
+/// (WASI does not have concept of absolute pathes).
+///
+/// Caller owns:
+///  * `handle` field,
+///  * if host OS is not WASI, also owns `path` field.
+pub fn resolveGlobalCacheDir(allocator: mem.Allocator, override_from_arg: ?[]const u8) !std.Build.Cache.Directory {
+    if (builtin.os.tag == .wasi) {
+        // Simplified logic, WASI does not have concept of absolute pathes.
+        if (override_from_arg) |override|
+            return .{
+                .path = override,
+                .handle = try fs.cwd().makeOpenPath(override, .{}),
+            }
+        else
+            return getWasiPreopen("/cache");
     }
 
-    return fs.getAppDataDir(allocator, appname);
+    const original_path = orig: {
+        if (override_from_arg) |override| break :orig try allocator.dupe(u8, override);
+
+        const override_from_env = try std.zig.EnvVar.ZIG_GLOBAL_CACHE_DIR.get(allocator);
+        if (override_from_env) |override| break :orig override;
+
+        const appname = "zig";
+
+        if (builtin.os.tag != .windows) {
+            if (std.zig.EnvVar.XDG_CACHE_HOME.getPosix()) |cache_root|
+                break :orig try fs.path.join(allocator, &[_][]const u8{ cache_root, appname })
+            else if (std.zig.EnvVar.HOME.getPosix()) |home|
+                break :orig try fs.path.join(allocator, &[_][]const u8{ home, ".cache", appname });
+        }
+
+        break :orig try fs.getAppDataDir(allocator, appname);
+    };
+
+    std.log.err("original_path = {s}", .{original_path});
+    const absolute_path = if (fs.path.isAbsolute(original_path))
+        original_path
+    else absolute: {
+        std.log.err("original_path is not absolute! Converting to absolute...", .{});
+        defer allocator.free(original_path);
+
+        const cwd_path = try std.process.getCwdAlloc(allocator);
+        defer allocator.free(cwd_path);
+        std.log.err("cwd_path = {s}", .{cwd_path});
+
+        std.log.err("Converting...", .{});
+        break :absolute try fs.path.resolve(allocator, &.{ cwd_path, original_path });
+    };
+    std.log.err("absolute_path = {s}", .{absolute_path});
+
+    return .{
+        .path = absolute_path,
+        .handle = try fs.cwd().makeOpenPath(absolute_path, .{}),
+    };
 }
 
 /// Similar to std.fs.path.resolve, with a few important differences:
diff --git a/src/main.zig b/src/main.zig
index 61a97de3f5..6f013e3451 100644
--- a/src/main.zig
+++ b/src/main.zig
@@ -54,7 +54,7 @@ pub fn wasi_cwd() std.os.wasi.fd_t {
     return cwd_fd;
 }
 
-fn getWasiPreopen(name: []const u8) Compilation.Directory {
+pub fn getWasiPreopen(name: []const u8) Compilation.Directory {
     return .{
         .path = name,
         .handle = .{
@@ -894,7 +894,7 @@ fn buildOutputType(
     var test_name_prefix: ?[]const u8 = null;
     var test_runner_path: ?[]const u8 = null;
     var override_local_cache_dir: ?[]const u8 = try EnvVar.ZIG_LOCAL_CACHE_DIR.get(arena);
-    var override_global_cache_dir: ?[]const u8 = try EnvVar.ZIG_GLOBAL_CACHE_DIR.get(arena);
+    var override_global_cache_dir: ?[]const u8 = null;
     var override_lib_dir: ?[]const u8 = try EnvVar.ZIG_LIB_DIR.get(arena);
     var clang_preprocessor_mode: Compilation.ClangPreprocessorMode = .no;
     var subsystem: ?std.Target.SubSystem = null;
@@ -2699,22 +2699,7 @@ fn buildOutputType(
     };
     defer zig_lib_directory.handle.close();
 
-    var global_cache_directory: Compilation.Directory = l: {
-        if (override_global_cache_dir) |p| {
-            break :l .{
-                .handle = try fs.cwd().makeOpenPath(p, .{}),
-                .path = p,
-            };
-        }
-        if (native_os == .wasi) {
-            break :l getWasiPreopen("/cache");
-        }
-        const p = try introspect.resolveGlobalCacheDir(arena);
-        break :l .{
-            .handle = try fs.cwd().makeOpenPath(p, .{}),
-            .path = p,
-        };
-    };
+    var global_cache_directory: Compilation.Directory = try introspect.resolveGlobalCacheDir(arena, override_global_cache_dir);
     defer global_cache_directory.handle.close();
 
     if (linker_optimization) |o| {
@@ -4740,7 +4725,7 @@ fn cmdBuild(gpa: Allocator, arena: Allocator, args: []const []const u8) !void {
 
     var build_file: ?[]const u8 = null;
     var override_lib_dir: ?[]const u8 = try EnvVar.ZIG_LIB_DIR.get(arena);
-    var override_global_cache_dir: ?[]const u8 = try EnvVar.ZIG_GLOBAL_CACHE_DIR.get(arena);
+    var override_global_cache_dir: ?[]const u8 = null;
     var override_local_cache_dir: ?[]const u8 = try EnvVar.ZIG_LOCAL_CACHE_DIR.get(arena);
     var override_build_runner: ?[]const u8 = try EnvVar.ZIG_BUILD_RUNNER.get(arena);
     var child_argv = std.ArrayList([]const u8).init(arena);
@@ -4933,13 +4918,7 @@ fn cmdBuild(gpa: Allocator, arena: Allocator, args: []const []const u8) !void {
     });
     child_argv.items[argv_index_build_file] = build_root.directory.path orelse cwd_path;
 
-    var global_cache_directory: Compilation.Directory = l: {
-        const p = override_global_cache_dir orelse try introspect.resolveGlobalCacheDir(arena);
-        break :l .{
-            .handle = try fs.cwd().makeOpenPath(p, .{}),
-            .path = p,
-        };
-    };
+    var global_cache_directory: Compilation.Directory = try introspect.resolveGlobalCacheDir(arena, override_global_cache_dir);
     defer global_cache_directory.handle.close();
 
     child_argv.items[argv_index_global_cache_dir] = global_cache_directory.path orelse cwd_path;
@@ -5365,7 +5344,6 @@ fn jitCmd(
         .ReleaseFast;
     const strip = optimize_mode != .Debug;
     const override_lib_dir: ?[]const u8 = try EnvVar.ZIG_LIB_DIR.get(arena);
-    const override_global_cache_dir: ?[]const u8 = try EnvVar.ZIG_GLOBAL_CACHE_DIR.get(arena);
 
     var zig_lib_directory: Compilation.Directory = if (override_lib_dir) |lib_dir| .{
         .path = lib_dir,
@@ -5377,13 +5355,7 @@ fn jitCmd(
     };
     defer zig_lib_directory.handle.close();
 
-    var global_cache_directory: Compilation.Directory = l: {
-        const p = override_global_cache_dir orelse try introspect.resolveGlobalCacheDir(arena);
-        break :l .{
-            .handle = try fs.cwd().makeOpenPath(p, .{}),
-            .path = p,
-        };
-    };
+    var global_cache_directory: Compilation.Directory = try introspect.resolveGlobalCacheDir(arena, null);
     defer global_cache_directory.handle.close();
 
     var thread_pool: ThreadPool = undefined;
@@ -6910,7 +6882,7 @@ fn cmdFetch(
     const work_around_btrfs_bug = native_os == .linux and
         EnvVar.ZIG_BTRFS_WORKAROUND.isSet();
     var opt_path_or_url: ?[]const u8 = null;
-    var override_global_cache_dir: ?[]const u8 = try EnvVar.ZIG_GLOBAL_CACHE_DIR.get(arena);
+    var override_global_cache_dir: ?[]const u8 = null;
     var debug_hash: bool = false;
     var save: union(enum) {
         no,
@@ -6967,13 +6939,7 @@ fn cmdFetch(
     const root_prog_node = progress.start("Fetch", 0);
     defer root_prog_node.end();
 
-    var global_cache_directory: Compilation.Directory = l: {
-        const p = override_global_cache_dir orelse try introspect.resolveGlobalCacheDir(arena);
-        break :l .{
-            .handle = try fs.cwd().makeOpenPath(p, .{}),
-            .path = p,
-        };
-    };
+    var global_cache_directory: Compilation.Directory = try introspect.resolveGlobalCacheDir(arena, override_global_cache_dir);
     defer global_cache_directory.handle.close();
 
     var job_queue: Package.Fetch.JobQueue = .{
diff --git a/src/print_env.zig b/src/print_env.zig
index b51423656b..c50c5f09c0 100644
--- a/src/print_env.zig
+++ b/src/print_env.zig
@@ -15,7 +15,7 @@ pub fn cmdEnv(arena: Allocator, args: []const []const u8, stdout: std.fs.File.Wr
 
     const zig_std_dir = try std.fs.path.join(arena, &[_][]const u8{ zig_lib_directory.path.?, "std" });
 
-    const global_cache_dir = try introspect.resolveGlobalCacheDir(arena);
+    const global_cache_dir = try introspect.resolveGlobalCacheDir(arena, null);
 
     const host = try std.zig.system.resolveTargetQuery(.{});
     const triple = try host.zigTriple(arena);

As a commit (not very readable on GitHub, IIRC it uses Myers, I'm using histogram diff): https://github.com/BratishkaErik/zig/commit/323489c0e8a1a2c45b6fd3ca1614bd2cff145ae2

Output after this "patch":

$ ~/github.com/zig/build/debug/bin/zig build --global-cache-dir zig-gg                                                          
error: original_path = zig-gg
error: original_path is not absolute! Converting to absolute...
error: cwd_path = /home/bratishkaerik/github.com/zig-bug-local-cache-dep
error: Converting...
error: absolute_path = /home/bratishkaerik/github.com/zig-bug-local-cache-dep/zig-gg
freetype_dep.path: /home/bratishkaerik/github.com/zig-bug-local-cache-dep/zig-gg/p/1220b81f6ecfb3fd222f76cf9106fecfa6554ab07ec7fdc4124b9bb063ae2adf969d

$ ~/github.com/zig/build/debug/bin/zig build --global-cache-dir "/home/bratishkaerik/github.com/zig-bug-local-cache-dep/zig-gg/"
error: original_path = /home/bratishkaerik/github.com/zig-bug-local-cache-dep/zig-gg/
error: absolute_path = /home/bratishkaerik/github.com/zig-bug-local-cache-dep/zig-gg/
freetype_dep.path: /home/bratishkaerik/github.com/zig-bug-local-cache-dep/zig-gg/p/1220b81f6ecfb3fd222f76cf9106fecfa6554ab07ec7fdc4124b9bb063ae2adf969d

I don't know if zig fetch or other commands could have the same issue, thus far I only ran into it with zig build.

If they have such issue, this patch should also already fix them.

BratishkaErik avatar May 26 '24 19:05 BratishkaErik

upon reflecting, this isn't the proper solution but just an attempt at fixing symptoms caused by another bug.

Jan200101 avatar May 29 '24 06:05 Jan200101