chore(ffi): avoid hardcoding clang version #4405
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4405 +/- ##
=======================================
Coverage 85.27% 85.28%
=======================================
Files 282 282
Lines 31209 31209
=======================================
+ Hits 26615 26617 +2
+ Misses 4594 4592 -2 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Sure 😅 I'll ask someone who has more Android knowledge to review this, as I'm not sure why the change is needed (supposedly NDK_CLANG_VERSION` is not always set anymore?).
If you wanted to go the extra mile, we could introduce a new tiny crate for building this, that would contain the code for the workaround setup. It seemed fine to duplicate when it was one function-long, but now that it seems to become a bit more involved, sharing code would make sense IMO.
| let toolchain_path = cc | ||
| .ancestors() | ||
| .nth(2) |
There was a problem hiding this comment.
Can you include an example of what cc would look like, so we understand a bit better why nth(2) is the right thing to do?
| // Configure rust to statically link against the `libclang_rt.builtins` supplied with clang. | ||
|
|
||
| // cargo-ndk sets CC_x86_64-linux-android to the path to `clang`, within the Android NDK. | ||
| let cc = PathBuf::from(env::var("CC_x86_64-linux-android").expect("CC_x86_64-linux-android not set")); |
There was a problem hiding this comment.
Can this variable be called clang_path?
| println!("cargo:rustc-link-lib=static=clang_rt.builtins-x86_64-android"); | ||
| } | ||
| } | ||
|
|
||
| /// Run the clang binary at `clang_path`, and return its major version number | ||
| fn get_clang_major_version(clang_path: impl AsRef<OsStr>) -> String { |
There was a problem hiding this comment.
Since this is called only once, I don't think we need a generic impl AsRef<...> parameter here 👀
I don't think it was ever set, unless you knew about it and set it manually. |
It would, but ugh my life is running short right now. I could do with just shipping this :/ |
|
Sorry, I wasn't able to take a good look at this PR until now: I'm not really fond of this solution, since we went from something that would work most of the time if you had the If we do this change it'll break the setups of everyone building the SDK Kotlin bindings up until now if I'm not mistaken, including the https://github.com/matrix-org/matrix-rust-components-kotlin repo, so I think it should be well documented in the bindings crate README file, with some example (same for the crypto-ffi crate)... as should have been the workaround already implemented by me, so I apologise for that 😓 . Maybe a 'Breaking' changelog entry in the commit message would be good too. |
No, the whole point is that @jmartinesp: would you mind giving it a go and confirming that it does, in fact, work for you? |
Oh, TIL! Then I retract everything I said. I already started a build in the bindings repo to test this out: https://github.com/matrix-org/matrix-rust-components-kotlin/actions/runs/12295378552/job/34312142906 |
| // cargo-ndk sets CC_x86_64-linux-android to the path to `clang`, within the | ||
| // Android NDK. |
There was a problem hiding this comment.
I completely missed this comment 🤦 .
Update the workaround for rust-lang/rust#109717 to avoid hardcoding the clang version; instead, run `clang -dumpversion` to figure it out. While we're there, use the `CC_x86_64-linux-android` env var, which should point to clang, rather than relying on `ANDROID_NDK_HOME` to be set.
richvdh
force-pushed
the
rav/no_hardcode_clang
branch
from
3f15bca to
909fa56
Compare
Update the workaround for rust-lang/rust#109717 to avoid hardcoding the clang version; instead, run
clang -dumpversionto figure it out.While we're there, use the
CC_x86_64-linux-androidenv var, which should point to clang, rather than relying onANDROID_NDK_HOMEto be set.