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

linkers: Disable -rpath-link with ld.zigcc #13493

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

RossComputerGuy
Copy link
Contributor

@RossComputerGuy RossComputerGuy commented Jul 31, 2024

Requires #12293

Zig issue ziglang/zig#18713

Draft until I figure out how to test this with Nix

Comment on lines +708 to +709
# NOTE: Remove the zigcc check once zig support "-rpath-link"
# See https://github.com/ziglang/zig/issues/18713
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we grep the help output from zig to know if it supports this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Or we can do a version check

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Version check yes, help check would probably be slower and might display the wrong thing (not 100% sure).

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm I guess support for this flag hasn't been planned, so the id check makes sense for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah hence the comment with the link. We can change it once the flag exists in a release version.

@RossComputerGuy
Copy link
Contributor Author

Looks like the tests are working with this PR.

@RossComputerGuy RossComputerGuy marked this pull request as ready for review August 1, 2024 04:52
@tristan957
Copy link
Contributor

So we need to merge this PR into the other one?

@RossComputerGuy
Copy link
Contributor Author

Yeah, probably.

@eli-schwartz
Copy link
Member

What we really need here is to have a single PR that contains all the necessary changes:

And a second PR that adds zig to the CI images without adding it to the CI workflows, see #12953 again.

Currently everything is spread out quite a bit and it's difficult to get a handle on whether it all works.

@RossComputerGuy
Copy link
Contributor Author

What we really need here is to have a single PR that contains all the necessary changes

I can work on that this evening. I already have a branch that I was testing with that has this PR and the main one merged in.

@RossComputerGuy
Copy link
Contributor Author

@eli-schwartz I've opened #13515, going to let CI run and test locally when I have the time. Will fix things tonight if there's any problems.

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.

3 participants