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

Store Build relative information in LazyPath #19313

Closed
ikskuh opened this issue Mar 15, 2024 · 4 comments · Fixed by #19597
Closed

Store Build relative information in LazyPath #19313

ikskuh opened this issue Mar 15, 2024 · 4 comments · Fixed by #19597
Labels
accepted This proposal is planned. breaking Implementing this issue could cause existing code to no longer compile or have different behavior. contributor friendly This issue is limited in scope and/or knowledge of Zig internals. proposal This issue suggests modifications. If it also has the "accepted" label then it is planned. zig build system std.Build, the build runner, `zig build` subcommand, package management
Milestone

Comments

@ikskuh
Copy link
Contributor

ikskuh commented Mar 15, 2024

Right now, .{ .path = "xyz" } is both (a bit) clunky and is missing information about the std.Build it is meant to be relative to.

I often use cwd_relative now and just store an absolute path computed with @src(), but i don't think this is the right solution here.

Thus, i propose to change

path: []const u8,

to

path: struct {
  rel: []const u8, 
  root: *std.Build,
}

and to make the usage easier, to introduce

pub fn path(build: *std.Build, path: []const u8) LazyPath {
    return .{ .path = .{ .root = build, .rel = path } };
}

Which would replace .{ .path = "xyz" } with b.path("xyz") which is even shorter than before, but packs more information (which is sometimes required in builds when handling LazyPaths over several build scripts)

@castholm
Copy link
Contributor

castholm commented Mar 15, 2024

I would be in favor of this. .{ .path = "xyz" } is currently understood to be relative to whichever builder is assigned to the owner field of Step, which can become a bit footgunny when you start involving dependencies. For example, if you for some reason copy a regular LazyPath.path from a module or an artifact exposed by a dependency to one of your own steps, you will most likely end up with incorrect results (I might actually need to go back and double check that some of my open build system PRs work across dependency boundaries 😅)

Were this to be accepted, another bonus is that we can get rid of the LazyPath.dependency union field because it's effectively identical to the union field you're suggesting.

@lacc97
Copy link
Contributor

lacc97 commented Mar 17, 2024

This is closely related to #16067 and #18450. I've been thinking about this and I believe these could be solved if LazyPath looked like:

const LazyPath = struct {
    /// The owner builder object.
    build: *std.Build,

    /// A path relative to `root`.
    path: []const u8,

    ///
    root: union(enum) {
        /// Indicates the path is evaluated relative to the build root directory.
        build_relative,

        /// Indicates the path is evaluated relative to the current working directory.
        cwd_relative,

        /// Indicates the path is evaluated relative to the generated cache path.
        generated: *const std.Build.GeneratedFile,
    },
};

Firstly, this eliminates the generated_dirname and dependency union fields which were a bit hacky.

For #16067, implementing fn subpath(lp: LazyPath, rp: [] const u8) LazyPath is straightforward by manipulation of the path field, and having access to the *Build means we also have access to an Allocator for path joining.

For this issue (#19313), the build_relative union field should take care of it.

For #18450 (partly), checking the path field should be enough to prevent escaping root (unless there's symlink shenanigans afoot of course).

@ikskuh
Copy link
Contributor Author

ikskuh commented Mar 18, 2024

This is closely related to #16067 and #18450. I've been thinking about this and I believe these could be solved if LazyPath looked like:

const LazyPath = struct {
    /// The owner builder object.
    build: *std.Build,

    /// A path relative to `root`.
    path: []const u8,

    ///
    root: union(enum) {
        /// Indicates the path is evaluated relative to the build root directory.
        build_relative,

        /// Indicates the path is evaluated relative to the current working directory.
        cwd_relative,

        /// Indicates the path is evaluated relative to the generated cache path.
        generated: *const std.Build.GeneratedFile,
    },
};

This looks like a really fine solution to the problem space! A regular generated file source has path="." then with generated set :)

I really like this!

@andrewrk andrewrk added contributor friendly This issue is limited in scope and/or knowledge of Zig internals. breaking Implementing this issue could cause existing code to no longer compile or have different behavior. proposal This issue suggests modifications. If it also has the "accepted" label then it is planned. accepted This proposal is planned. zig build system std.Build, the build runner, `zig build` subcommand, package management labels Apr 7, 2024
@andrewrk andrewrk added this to the 0.13.0 milestone Apr 7, 2024
@andrewrk
Copy link
Member

andrewrk commented Apr 7, 2024

This is a pretty important change, and I think it's worth making one more API break before tagging 0.12.

@andrewrk andrewrk modified the milestones: 0.13.0, 0.12.0 Apr 7, 2024
andrewrk added a commit that referenced this issue Apr 10, 2024
This adds the *std.Build owner to LazyPath so that lazy paths returned
from a dependency can be used in the application without friction or
footguns.

closes #19313
andrewrk added a commit that referenced this issue Apr 10, 2024
This adds the *std.Build owner to LazyPath so that lazy paths returned
from a dependency can be used in the application without friction or
footguns.

closes #19313
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted This proposal is planned. breaking Implementing this issue could cause existing code to no longer compile or have different behavior. contributor friendly This issue is limited in scope and/or knowledge of Zig internals. proposal This issue suggests modifications. If it also has the "accepted" label then it is planned. zig build system std.Build, the build runner, `zig build` subcommand, package management
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants