Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

eliminate absolute paths from the build system #18450

Open
Tracked by #8
andrewrk opened this issue Jan 4, 2024 · 1 comment
Open
Tracked by #8

eliminate absolute paths from the build system #18450

andrewrk opened this issue Jan 4, 2024 · 1 comment
Labels
enhancement Solving this issue will likely involve adding new logic or components to the codebase. standard library This issue involves writing Zig code for the standard library. zig build system std.Build, the build runner, `zig build` subcommand, package management
Milestone

Comments

@andrewrk
Copy link
Member

andrewrk commented Jan 4, 2024

Absolute paths are generally problematic:

  • not portable
  • subject to inter-process race conditions
  • emplace artificial limitations on lengths of file system paths
  • security concerns; incompatible with being explicitly given the available directory handles to work with

For general improvement in robustness as well as progress towards #14286, this issue is to eliminate all absolute paths from the zig build system. Here are some example violations:

const output_path = try b.cache_root.join(arena, &output_components);
placeholder.output.generated_file.path = output_path;

placeholder.output.generated_file.path = try cache_root.join(arena, &.{
"o", digest, placeholder.output.basename,
});
}
if (captured_stdout) |output| {
output.generated_file.path = try cache_root.join(arena, &.{
"o", digest, output.basename,
});
}
if (captured_stderr) |output| {
output.generated_file.path = try cache_root.join(arena, &.{
"o", digest, output.basename,
});

file.generated_file.path = try b.cache_root.join(b.allocator, &.{
"o", &digest, file.sub_path,
});

There are plenty more.

Instead, this API should be moved from the compiler implementation (Package.Path) to std.Cache.Path:

zig/src/Package.zig

Lines 6 to 10 in 501a235

pub const Path = struct {
root_dir: Cache.Directory,
/// The path, relative to the root dir, that this `Path` represents.
/// Empty string means the root_dir is the path.
sub_path: []const u8 = "",

Then, Path objects should be passed around rather than strings. This will improve the correctness of a bunch of stuff as well as reduce the amount of unnecessary string manipulation that is happening, and make it easier to implement detection of going outside a directory root.

@andrewrk andrewrk added enhancement Solving this issue will likely involve adding new logic or components to the codebase. standard library This issue involves writing Zig code for the standard library. zig build system std.Build, the build runner, `zig build` subcommand, package management labels Jan 4, 2024
@andrewrk andrewrk added this to the 0.13.0 milestone Jan 4, 2024
@ikskuh
Copy link
Contributor

ikskuh commented Jan 5, 2024

Some general comment from what i've figured:
LazyPath is missing a crucial component now that the package manger is there: The path field is relative to some build.zig, but we do not store the relevant builder.

I think we should add some api like fn path(b: *Build, rel: []const u8) LazyPath which constructs a path that is guaranteed to be relative to the given builder.

This way we can safely transport LazyPaths between dependency edges without confusing the build root

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Solving this issue will likely involve adding new logic or components to the codebase. standard library This issue involves writing Zig code for the standard library. zig build system std.Build, the build runner, `zig build` subcommand, package management
Projects
None yet
Development

No branches or pull requests

2 participants