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

Update Android compiler detection for NDK r25 #728

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

chriswailes
Copy link

This commit updates the compiler detection logic for the Android platform to work with NDK r25. This code is largely identical to the corresponding code in rustc/src/bootstrap/cc_detect.rs.

This commit updates the compiler detection logic for the Android
platform to work with NDK r25.  This code is largely identical to the
corresponding code in rustc/src/bootstrap/cc_detect.rs.
@chriswailes
Copy link
Author

It occurs to me that if this is committed then developers using older versions of rustc (and targeting the corresponding Android NDK) but a new version of cc will encounter errors during compiler detection as the new paths will be returned. Ideally we would check on the compiler version to determine which version of the names to return. We could either do this using the rustversion crate (proc-macro) or using the rustc_version check (either in build.rs or in lib.rs directly as it is executed at build time as well).

Is there a preference between these options?

@thomcc thomcc added the O-android Operating system: Android label Oct 26, 2022
@thomcc
Copy link
Member

thomcc commented Oct 29, 2022

Is there a preference between these options?

Getting the version by parsing the result of rustc --version is very simple (like 10-20loc tops) so I'd definitely prefer manually doing that rather than pulling in dependencies for this (one of those pulls in dependencies of its own for e.g. semver parsing, and the other is a proc macro).

You should probably get the path to rustc via the RUSTC env var, falling back to rustc if it's unset. I guess if that fails I might default to assuming a new version of rustc, to avoid a situation where the handling of old rustc breaks things for users of new rustc.

@thomcc
Copy link
Member

thomcc commented Oct 29, 2022

Note to self: this shouldn't get merged until after the discussion in rust-lang/rust#103673 is fully resolved.

@chriswailes
Copy link
Author

Thanks for the feedback. I'm in the middle of putting out some fires but I'll try and get to this ASAP.

@Rustin170506
Copy link
Member

@chriswailes Could we possibly move forward on this PR? It seems rust-lang/rust#105716 landed again.

@Rustin170506
Copy link
Member

@thomcc rust-lang/rust#103673 was closed now. Could you please take a look at this PR?

@NobodyXu
Copy link
Collaborator

This PR would need a rebase

@torokati44
Copy link

Do you think this could fix ruffle-rs/ruffle-android#62 ? 🤔

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
O-android Operating system: Android
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants