-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
base: master
Are you sure you want to change the base?
Conversation
# NOTE: Remove the zigcc check once zig support "-rpath-link" | ||
# See https://github.com/ziglang/zig/issues/18713 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Looks like the tests are working with this PR. |
So we need to merge this PR into the other one? |
Yeah, probably. |
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. |
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. |
@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. |
Requires #12293
Zig issue ziglang/zig#18713
Draft until I figure out how to test this with Nix