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

Fix Android clang compiler path when building with Windows host #489

Merged
merged 2 commits into from
Apr 22, 2020

Conversation

Osspial
Copy link
Contributor

@Osspial Osspial commented Apr 21, 2020

On Windows, the Android NDK provides the %ANDROID_TARGET%-clang executable as a .cmd file rather than an .exe file. std::process::Command can only resolve .exe commands when the command extension isn't explicitly provided (see rust-lang/rust#37519 (comment)), so cc-rs mistakenly thinks the %ANDROID_TARGET%-clang compiler isn't installed on the system even when it's in the path. This PR fixes that.

@alexcrichton
Copy link
Member

To confirm, this has always been the case, right? Should we perhaps check for .exe and fall back to .cmd if it's available?

@Osspial
Copy link
Contributor Author

Osspial commented Apr 21, 2020

As far as I understand this has always been the case, but there's no harm in checking for the .exe file. I've updated the code to do so.

@alexcrichton
Copy link
Member

Thanks! Actually though while you're at it, this is something that I missed previously. Could you replace .spawn() with .output()? Otherwise I think this runs a risk of creating a zombie process.

@alexcrichton
Copy link
Member

Ah it's ok I'll tweak that locally

@alexcrichton alexcrichton merged commit 4c6e7c6 into rust-lang:master Apr 22, 2020
@Osspial
Copy link
Contributor Author

Osspial commented Apr 22, 2020

It looks like 4469060 changed the check at lib.rs:1994 from is_err to is_ok, which broke this fix.

@alexcrichton
Copy link
Member

Oops sorry about that! I mistakenly assumed which compiler was being tested. I pushed up a new release which I think should fix this.

@Osspial
Copy link
Contributor Author

Osspial commented Apr 22, 2020

Looks like that's fixed things. Thanks!

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