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

Pattern match the linux x86_64 clang directory #975

Merged
merged 2 commits into from
May 17, 2024
Merged

Conversation

dangeross
Copy link
Collaborator

@dangeross dangeross commented May 15, 2024

This PR uses glob in the build to pattern match the linux x86_64 clang directory

@dangeross dangeross requested review from roeierez and JssDWt May 15, 2024 11:34
Copy link
Member

@roeierez roeierez left a comment

Choose a reason for hiding this comment

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

LGTM - seems like a robust way to fix different paths in different NDKs.
Small NIT.

println!("cargo:rustc-link-lib=static=clang_rt.builtins-x86_64-android");
} else {
panic!("Path {linkpath} not exists. Try setting the NDK_CLANG_VERSION environment variable.");
match glob(&linux_x86_64_lib_pattern)?.last() {
Copy link
Member

Choose a reason for hiding this comment

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

NIT: Since we already panic at this function perhaps using unwrap here instead of changing the method prototype?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

Copy link
Member

@roeierez roeierez left a comment

Choose a reason for hiding this comment

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

LGTM

println!("cargo:rustc-link-lib=static=clang_rt.builtins-x86_64-android");
} else {
panic!("Path {linkpath} not exists. Try setting the NDK_CLANG_VERSION environment variable.");
match glob(&linux_x86_64_lib_pattern).unwrap().last() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks good. I have a nit that may not be useful. This might not always take the latest clang version if multiple are available. (Is that even ever the case?). (Do we even need the latest version?). Anyway, v9.0.0 will be the 'last' if v10.0.0 is also available, because the results from the glob are sorted alphabetically.

Copy link
Contributor

Choose a reason for hiding this comment

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

The clang version is in the double-digits, so that won't be a problem for a while.

Copy link
Contributor

Choose a reason for hiding this comment

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

The clang version is in the double-digits, so that won't be a problem for a while.

Yeah true only minor and patch versions maybe.

@dangeross dangeross merged commit 277e164 into main May 17, 2024
7 checks passed
@dangeross dangeross deleted the savage-build-glob branch May 17, 2024 07:22
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.

4 participants