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

Change nanobind linkage to response file approach on macOS #1638

Merged
merged 3 commits into from
Oct 24, 2023

Conversation

nicholasjng
Copy link
Contributor

This change needs bazelbuild/bazel#18952 to be merged first.

Fixes macOS linkage of GBM's nanobind bindings on macOS by supplying a linker response file instead of -undefined dynamic_lookup. The latter has since been deprecated on macOS.

@nicholasjng
Copy link
Contributor Author

This is blocked by the mentioned Bazel PR. I will check if I can implement this with the linker input suggested in #1636, but I will reopen this when we have a Bazel release supporting the linker input directly on the cc_library.

@nicholasjng nicholasjng reopened this Oct 20, 2023
@nicholasjng
Copy link
Contributor Author

Reopening, as Bazel 6.4.0 was released with the linker response file support merged in.

Give me some time to polish this again, I would request a review (hopefully) when I'm set :)

@nicholasjng nicholasjng force-pushed the nanobind-response-file branch from 2fe54cf to 92dca2f Compare October 23, 2023 09:56
@nicholasjng
Copy link
Contributor Author

nicholasjng commented Oct 23, 2023

Rebased on main, fixed the bazel_skylib checksum.

I think the changes from #1676 can be applied separately.

cc @dmah42

EDIT: Windows is failing, will investigate shortly.

EDIT2: windows-2019 does not have Bazel 6.4.0, bumping to latest (or merging #1676) should do the trick.

This change needs bazelbuild/bazel#18952 to be
merged first. Fixes macOS linkage of GBM's nanobind bindings on macOS by
supplying a linker response file instead of `-undefined dynamic_lookup`.

The latter has since been deprecated on macOS.
@nicholasjng nicholasjng force-pushed the nanobind-response-file branch from 92dca2f to ec025b4 Compare October 23, 2023 12:18
@nicholasjng
Copy link
Contributor Author

nicholasjng commented Oct 23, 2023

Thanks for the merge, rebased on current main.

What's your take on bumping Bazel dependencies? Some of them are quite stale, so I updated a few in this PR (not rules_python though, since that is misconfigured right now and raises an error in the latest v0.26.0, I'm on it).

EDIT: Seems windows GH runners have not upgraded to Bazel v6.4.0 yet.

@dmah42
Copy link
Member

dmah42 commented Oct 23, 2023

i have no concerns about bumping dependencies if they're stale. is this ready for me to re-review?

@nicholasjng
Copy link
Contributor Author

i have no concerns about bumping dependencies if they're stale. is this ready for me to re-review?

yes, all set, GitHub won't let me request an initial review, that's all :)

Re: the windows-latest build failure, that should be out of the way in the next actions/runners Windows image release, you can check the release page here.

@dmah42
Copy link
Member

dmah42 commented Oct 23, 2023

i have no concerns about bumping dependencies if they're stale. is this ready for me to re-review?

yes, all set, GitHub won't let me request an initial review, that's all :)

Re: the windows-latest build failure, that should be out of the way in the next actions/runners Windows image release, you can check the release page here.

should we go back to windows-2019 or would that not help? what's the timeline for the release? looks like the last one was 2 weeks ago.

@nicholasjng
Copy link
Contributor Author

should we go back to windows-2019 or would that not help? what's the timeline for the release? looks like the last one was 2 weeks ago.

That won't help, it was actually the failure on windows-2019 that made me assume we need latest. Timeline is probably anywhere within this week, cadence seems to be <every 2 weeks.

@nicholasjng
Copy link
Contributor Author

New windows-latest version is out, with Bazel 6.4.0: https://github.com/actions/runner-images/releases/tag/win22%2F20231023.1

Should be good to go now.

@dmah42 dmah42 merged commit e45585a into google:main Oct 24, 2023
53 of 60 checks passed
@nicholasjng nicholasjng deleted the nanobind-response-file branch April 15, 2024 16:46
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