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

std.Build.LazyPath: add sub path method #19024

Closed

Conversation

RossComputerGuy
Copy link
Contributor

Closes #16067

This implements joined lazy paths (sub path)

@RossComputerGuy RossComputerGuy force-pushed the feat/lazypath-join branch 2 times, most recently from 38ccae5 to 4ab7128 Compare February 21, 2024 03:49
@RossComputerGuy RossComputerGuy marked this pull request as ready for review February 21, 2024 15:53
@lacc97
Copy link
Contributor

lacc97 commented Feb 21, 2024

Wouldn't this make it really easy to dangle a LazyPath since the parent now requires a stable address?

@RossComputerGuy
Copy link
Contributor Author

Possibly, I could make the parent be a union similar to LazyPath so then this wouldn't be an issue. I can't make the parent be a LazyPath unless it was a pointer because recursion.

@lacc97
Copy link
Contributor

lacc97 commented Feb 21, 2024

How about adding a nullable subpath field to the LazyPath?

Ideally something like

const LazyPath = struct {
    subpath: ?[]const u8 = null,
    data: union(enum) {
        // ...
    },
};

but for backwards compatibility maybe flip it around and add the subpath to the other union fields instead?

Actually, wouldn't this only be needed for the generated* cases? Since the other union fields implicitly support subpaths given their payloads are already paths.

@RossComputerGuy
Copy link
Contributor Author

Probably, that sounds like it'll work the best. I'll try things out after work today.

@castholm
Copy link
Contributor

castholm commented Feb 21, 2024

Does this implementation prevent something like path.subPath("../../../../secret.txt") from reaching outside of the root of the underlying path (zig-cache/ for generated paths, the build root for regular paths, cwd for cwd-relative paths, etc.) in the same way that LazyPath.dirname does? Protecting against this was an important concern for dirname, so I figure it is just as important here.

You might want to take a look at the dirname issue #17411 and the related PR that implemented it for inspiration. A few tests that verify the correctness of the behavior would probably be good as well.

@RossComputerGuy
Copy link
Contributor Author

I don't think it does protect from that in its current state. I'll see if I can prevent escaping when I work on the changes tonight.

@RossComputerGuy RossComputerGuy force-pushed the feat/lazypath-join branch 3 times, most recently from 40d4668 to 61b72f8 Compare February 26, 2024 00:41
@RossComputerGuy
Copy link
Contributor Author

Looking at how much would have to be changed, this is too much for now. Gonna close this PR, if someone wishes to finish this then I recommend some discussion on how this should be implemented so it doesn't break everything.

@RossComputerGuy RossComputerGuy deleted the feat/lazypath-join branch March 7, 2024 21:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature Request: subpath derived FileSource in build.zig
3 participants