Skip to content

Make bindgen apple-aware - #179

Open
adincebic wants to merge 1 commit into
hermeticbuild:mainfrom
adincebic:adin/bindgen-apple-aware
Open

adincebic wants to merge 1 commit into
hermeticbuild:mainfrom
adincebic:adin/bindgen-apple-aware

Conversation

@adincebic

Copy link
Copy Markdown
Contributor

Use apple_support.run(...) when bindgen executes on macOS.

Comment thread rs/rules_rust_bindgen.bzl
bindgen="$1"
shift

resource_dir="$(xcrun clang -print-resource-dir)"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think ideally the apple_support toolchain would compute this resource-dir in the discovery repo rule and add that to the toolchain's args explicitly, instead of working around it here.

Failing that, I suppose we can bring back this logic which I wasn't a fan of.

@keith wdyt?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I guess it doesn't do that since we happily rely on the default? how does bindgen discover that? not from the active compiler?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yeah, right now apple_support relies on default sibling. Bindgen in rules_rust had logic above to try to guess it. In rules_rs version I removed it and expect cc toolchain to provide all flags it needs (explicit resource dir or -isystem etc)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't think it's safe to assume many toolchains will have that? quick check shows toolchains_llvm does pass it, but rules_cc, apple_support, and musl-toolchains don't. It would be a bit annoying for us since we'd have to compute it in a repo rule that we don't otherwise do stuff like this in, and then we'd have to relativize the non-hermetic path to one of the placeholder things variables, which also means then you'd have to be handling those replacements (since otherwise it would contain the absolute path to Xcode)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@keith both hermetic-llvm and toolchains_llvm have it, I think musl-toolchain is not very useful now that hermetic-llvm exists, and I'm not sure how much I care about the rules_cc auto toolchains...

But if you think computing it properly in apple_support is too annoying, I guess we can bring back the old detection method, it's just super janky...

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't think that version works for non-hermetic toolchains since all_files doesn't contain those files either?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

so has this never worked with rules_rust? @adincebic do you know?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think it did, let me check and report back.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

btw i was thinking, what if you configure your own bindgen toolxhain that invokes xcrun in a wrapper and then execs underlying bindgen. could even be a wrapper peocided by apple_support that you compose. feels a bit cleaner to me than having rust learn about this?

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