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

Use -Xlinker for -install_name #343

Merged
merged 1 commit into from
Sep 9, 2024
Merged

Conversation

fmeum
Copy link
Contributor

@fmeum fmeum commented Sep 4, 2024

This supports , in runtime_solib_name.

This supports `,` in `runtime_solib_name`.
@fmeum
Copy link
Contributor Author

fmeum commented Sep 4, 2024

Would it be possible to get a release after this has been merged? This would unblock enabling C++ remote execution tests for macOS in Bazel (bazelbuild/bazel#23085). The PR currently patches in this change, but I'm not sure whether it will be considered mergeable in this state.

@keith
Copy link
Member

keith commented Sep 9, 2024

when do these paths have commas? in general we preferred the comma version because bazel was doing weirdo stuff like uniquing linkopts and therefore dropping the -Xlinker arg, i think this is probably an obsolete specific concern, but that was the general idea

@fmeum
Copy link
Contributor Author

fmeum commented Sep 9, 2024

Target names with commas can trigger this (there's a Bazel test for this case). The feature above this one already uses -Xlinker. If deduplication doesn't work, that sounds like a bug we should solve regardless.

@keith
Copy link
Member

keith commented Sep 9, 2024

fwiw we tried to fix that but weren't able to land a pr upstream

@keith keith merged commit 07dd08d into bazelbuild:master Sep 9, 2024
10 checks passed
@fmeum
Copy link
Contributor Author

fmeum commented Sep 9, 2024

Can you point me to the PR?

@fmeum fmeum deleted the xlinker-installname branch September 9, 2024 18:57
@keith
Copy link
Member

keith commented Sep 9, 2024

see the deduplication stuff in this issue bazelbuild/bazel#16939 (comment)

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.

2 participants