Skip to content

make CompileStep.addRPath preserve relative paths #15907

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

make CompileStep.addRPath preserve relative paths #15907

wants to merge 1 commit into from

Conversation

@ghost
Copy link

@ghost ghost commented May 30, 2023

Closes #15849.

andrewrk
andrewrk requested changes Jun 13, 2023
View reviewed changes
Copy link
Member

@andrewrk andrewrk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will make rpaths relative to the build root (i.e. to the directory containing build.zig). Is that what is desired? I think it should instead be relative to the installation prefix, no?

lib/std/Build/Step/Compile.zig Outdated
};

zig_args.appendAssumeCapacity(rpath.getPath2(b, step));
std.debug.print("path is {s}\n", .{rpath.getPathRelative(b, step)});
Copy link
Member

@andrewrk andrewrk Jun 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

stray debug print

Copy link
Author

@ghost ghost Jun 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, fixed that.

@ghost
Copy link
Author

ghost commented Jun 14, 2023

🤷 this PR's purpose was only to make CompileStep.adRPath consistent with the -rpath flag to build-exe. I have no idea what behavior is desirable here, but if relative paths are being interpreted relative to the wrong root, isn't that a separate issue?

@hazeycode
Copy link
Contributor

hazeycode commented Jul 10, 2023 •
edited
Loading

There is also an issue resolving $ORIGIN on linux, I presume that this is what should be used if one wants to use a path relative to the installation dir?
And a similar issue (resolving @executable_path on Darwin) that was solved by @kubkon here: #15061

I don't know how this stuff should work.

@ghost
Copy link
Author

ghost commented Jul 10, 2023

Alright. Well, I'm not going to do any more work on this. Others feel free to add commits or close.

@hazeycode
Copy link
Contributor

hazeycode commented Jul 10, 2023

Alright. Well, I'm not going to do any more work on this. Others feel free to add commits or close.

I think what you have here is correct. But if it turns out to not be aligned with whatever the correct behavior is holistically, then I'd be happy to pick it up. I don't know what that is so hopefully someone else can clarify.

@ghost ghost closed this by deleting the head repository Dec 7, 2023
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

CompileStep.addRPath always generates absolute paths

2 participants

@hazeycode @andrewrk