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

Be more stringent about handling Android NDK. #808

Merged
merged 1 commit into from
May 18, 2023

Conversation

jfgoog
Copy link
Contributor

@jfgoog jfgoog commented May 17, 2023

Wrapper scripts for the NDK have the format <target>-<clang>, so when checking to see if the filename contains "android" (which will be part of the target), split off the target part of the filename and only check that, instead of checking the entire filename.

This is a very minor improvement, but it is more correct and does have a practical benefit for us on the Android toolchain team: Our build process for rustc uses wrapper scripts that include the target in their name, and the cc crate is misidentifying them as being from the NDK and causing our Windows build to fail without a workaround.

Copy link
Contributor

@ChrisDenton ChrisDenton left a comment

Choose a reason for hiding this comment

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

This seems reasonable to me. Though could you squish the commits?

Wrapper scripts for the NDK have the format <target>-<clang>, so
when checking to see if the filename contains "android" (which will be
part of the target), split off the target part of the filename and only
check that, instead of checking the entire filename.

This is a very minor improvement, but it is more correct and does have a
practical benefit for us on the Android toolchain team: Our build
process for rustc uses wrapper scripts that include the target in their
name, and the cc crate is misidentifying them as being from the NDK and
causing our Windows build to fail without a workaround.
@jfgoog
Copy link
Contributor Author

jfgoog commented May 18, 2023

Thanks! Squashed.

@ChrisDenton ChrisDenton merged commit 57853c4 into rust-lang:main May 18, 2023
16 checks passed
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.

None yet

2 participants