Use `llvm-readelf` to detect and bundle needed dynamic libs for Android #65

MarijnS95 · 2022-04-21T14:53:13Z

Fixes #47

Dynamic libraries that a Rust library links against while not being present on the target platform (libc++_shared.so and other custom libraries) need to be bundled in the APK specifically. This approach is adopted frm android-ndk-rs while switching from readelf to llvm-readelf.

xbuild/src/command/build.rs

MarijnS95 · 2022-04-21T14:55:48Z

xbuild/src/command/build.rs

+
+                    let ndk = env.android_ndk();
+
+                    let search_paths = []; // TODO: deps dir and everything from `cargo:rustc-link-search=`


I'll implement this in a separate PR later when needing it; for now I only really care about libc++_shared.so and would also like to bundle custom libs which are dlopened at runtime, also for a separate PR.

I guess it's probably easier to review the entire "dynamic library" handling at once? Also need some kind of test/example so we can check that it works for all platforms in ci.

I implemented the missing bits, will see about some CI action to test this functionality next.

xbuild/src/lib.rs

xcommon/src/llvm.rs

MarijnS95 · 2022-04-21T14:58:52Z

@dvc94ch How should we implement this for MacOS and iOS and the other targets?

dvc94ch · 2022-04-21T19:15:59Z

not sure, will have to try. at least for ios we don't need to bother, since rust doesn't support building dynamic libraries for ios. and I really don't have the time to mess about with major change proposals.

rust-lang/rust#95847

MarijnS95 · 2022-04-25T14:46:36Z

not sure, will have to try. at least for ios we don't need to bother, since rust doesn't support building dynamic libraries for ios. and I really don't have the time to mess about with major change proposals.

Thanks, as I'm not sure what you can do in terms of the other packaging targets x supports. Will probably need some additional refactoring though :/

xbuild/src/cargo/mod.rs

MarijnS95 · 2022-05-02T15:40:09Z

--------------------clang/llvm toolchain--------------------
clang                version             /usr/bin/clang
clang++              version             /usr/bin/clang++
llvm-ar              not found
llvm-lib             not found
llvm-readelf         not found
lld                  not found
lld-link             not found
lldb                 not found
lldb-server          not found

@dvc94ch Turns out GH actions CI doesn't ship with llvm-readelf, and I still don't like the Failed to run `the command` error with only a file not found message, as that really doesn't cover the load and may be interpreted as individual arguments being missing.

dvc94ch · 2022-05-02T15:46:51Z

should probably be llvm-readobj (llvm-readelf is just a symlink). you should setup ci like done by windows actions.

- uses: KyleMayes/install-llvm-action@v1
      with:
        version: 13
    - run: sudo ln -s llvm-ar-13 /usr/bin/llvm-lib
    - run: sudo ln -s lld-link-13 /usr/bin/lld-link

dvc94ch · 2022-05-02T15:57:33Z

Can't solve all problems at the same time. The clang/lldb/java/javac binaries are going to have to be preinstalled for the foreseeable future. It's possible we could try using rust-lld and llvm tools from rustup. Although it looks like it could disappear at any time since it's still a preview feature

MarijnS95 · 2022-05-02T17:53:51Z

should probably be llvm-readobj (llvm-readelf is just a symlink). you should setup ci like done by windows actions.

Oh okay, I thought it was already there. Will double-check the script and setup.

MarijnS95 · 2022-05-04T09:02:00Z

So it seems I've been reading the CI results for at least https://github.com/cloudpeers/xbuild/runs/6261865757?check_suite_focus=true all wrong.

LLVM is installed on Windows already (https://github.com/actions/virtual-environments/blob/main/images/win/Windows2022-Readme.md#language-and-runtime), and has the same files as the GH-actions installer, so that's not necessary (@dvc94ch how about the other platforms?);
The build succeeds there for Windows, with llvm-readobj. I've checked and the Windows installer indeed only provides llvm-readobj, not the readelf symlink (sensibly so, Windows doesn't use ELF);
The only build failing there is Ubuntu... Does it not have llvm-readobj but only llvm-readelf??

dvc94ch · 2022-05-04T09:34:00Z

Why are you symlinking llvm-readobj-12? Maybe you're trying to run a dangling symlink?

MarijnS95 · 2022-05-04T09:36:19Z

What's why are you symlinking llvm-readobj-12? Maybe you're trying to run a dangling symlink?

Shall I add a comment for these? Ubuntu only provides llvm-read{elf,obj}-1{0,1,2} so it won't be found when looking for llvm-readobj. Apparently llvm-readelf wasn't previously found on Ubuntu builds either.

MarijnS95 · 2022-05-04T10:03:52Z

Looks like the CI succeeds now, thanks to the symlinks 🥳

@dvc94ch I'll add a test for a small C++ file through the cc crate that requires libc++_shared.so on Android (as a third example?) then I think this is ready for a final round of review!

dvc94ch

Looking good, thanks for working on this!

dvc94ch · 2022-05-04T10:53:22Z

xcommon/src/llvm.rs

+pub fn list_needed_libs_recursively(
+    lib: &Path,
+    search_paths: &[&Path],
+    provided_libs_paths: &[&Path],


think we should change this logic. instead of a provided_libs_paths we should have a blacklist. this way we can maintain a text file of libraries not to include for each platform.

That sounds like a lot of work to update and maintain?

Also if we don't update this with new libraries, the logic will just copy the new libraries even when they are already available on the phone, making an APK unnecessarily bigger than it needs to be.

Note that none of the items in provided_libs_paths is used to copy or blacklist anything at all. That only happens for search_paths. The goal of provided_libs_paths is to at least validate that a library is available (without copying it!) so that we can otherwise provide an error instead of having the app fail at runtime.

(Except the libc++_shared.so edgecase - it's just that, a single edgecase)

for linux we already have an excludelist [0]. for android we can compute the exclude list by finding all libraries in provided_libs_path - libc++_shared.so. if it's in the exclude list we skip it otherwise we use search_paths to locate it and include it. for macos/windows we can compute the exclude list similarly.

[0] https://github.com/AppImage/pkg2appimage/blob/master/excludelist

well, it contains the library name not the path. the block list would be all file names with an so extension within provided paths. so something like filename(find provided_libs_paths | grep *.so)

in a sense it's already an excludelist, was just suggesting how to generalize it. it requries a bit more work but should be more generic.

ah, this is good enough for now, let's just merge it.

I think you have a fair point yes, but as pointed out below perhaps you weren't aware that provided_libs_paths is not included in search_paths currently.

Perhaps we can improve that as I quite like your suggestion. It is indeed already converted into an exclude list, with libc++_shared.so removed from it:

https://github.com/cloudpeers/xbuild/blob/30b0849d20b22825b233d19245a3f1c808e787dc/xcommon/src/llvm.rs#L22-L29

ah, this is good enough for now, let's just merge it.

Your call, we can also merge it now and improve in the future. I don't mind either way, I think we just misunderstood each others ideas in the beginning :)

xbuild/src/command/build.rs

dvc94ch · 2022-05-04T11:00:56Z

xcommon/src/llvm.rs

+                provided_libs_paths
+            } else {
+                search_paths
+            };


yep, this needs to go. you can compute the blocklist for android by searching the provided_libs and filtering for libc++_shared.so.

And then I copypaste that to a list somewhere... And document the manual commands/steps someone needs to go through to regenerate that list? Please no :(

Note also that your change implies that we'll add all the Android library paths from provided_libs_paths to search_paths which pretty much defeat the purpose since there's exactly one library from there that we need to copy, the rest should not be copied but only used to validate that all dynamic-link dependencies are met.

provided_libs_paths must already be included in search_paths. the search paths are anything added via -L, otherwise there's a bug.

They are not, and it should be by design?

well, maybe it's just named confusingly. but if the search path is the same that is passed to the linker, then any library that was linked must be in the search path. So this means that libc++_shared.so would be in both the search path and the provided path. So the provided path seems like it should be a subset of search path.

libc++ is a poor example, since we actually want to remove it from the provided path.

Yeah I think this does not match the search path passed to the linker. fn use_android_ndk() adds it to the rustflags but that isn't propagated to here.

What do you think, should we also parse -L from all the various rustflags sources (env var, .cargo/config.toml, etc) and pass those paths into this code as well? Then we can rework it into a blocklist as well.

We can also do this in a followup PR to not enlarge this one even further, though. I prefer keeping changes small as long as they don't actively and obviously break someones workflow or have outstanding issues/smells.

I think that's too fragile. Wondering about taking a different approach. How about we get the linker to tell us which libraries it needs from where? It looks like ld.lld supports a --Map=filepath argument that looks like it would give us just that.

xbuild/src/command/build.rs

dvc94ch · 2022-05-04T17:23:06Z

@MarijnS95 sorry for proposing all these last minute changes. Maybe if I'd have reviewed it earlier we could have been able to avoid it. To summarize my thoughs:

~~1. will merge #227 soon to prevent flutter regressions~~
2. detecting which libraries are needed could use the linker map instead of llvm-readobj + search path detection (search path detection seems error prone and incomplete)
3. all detected libraries that don't match a blacklist will be included
4. on android the blacklist is computed by searching the ndk's /usr/lib path

I think this is the most generic approach and should easily generalize to the other platforms. Sorry for making you jump through hoops. Once it's merged I'm commited to maintaining it, so we need to get the basic design right.

MarijnS95 · 2022-06-13T10:31:28Z

@dvc94ch Apologies for pretty much dropping the ball on this, been running into many other things that had higher prio and so this notification slipped to the end of my queue.

I totally understand where you're coming from and would love to do this differently too. The main pain-point being scanning various places for search paths and then perhaps not even maintaining the exact same order as ld.lld.

Anyway, since you approved this PR: Do you want to get this initial implementation in first, and let me rework it in a future PR - potentially removing lots of code again - or rather get that done first in this PR or a new PR? I'm already using this branch in automated Android CI builds to not be blocked, but I don't like being a downstream :)

dvc94ch · 2022-06-13T12:47:30Z

xbuild is currently on hold, due to lack of funding and it not being a priority right now.

I'm currently working on building the cloudpeer service. For this I started down the rabbit hole of building passwordless, an abstraction over roaming and platform authenticators.

This in turn will require a lot of android apis for the android platform authenticator. android.security.identity and android.hardware.biometric, which is were I plan to fit in some experimentation with aidl.

dvc94ch · 2022-06-13T13:37:58Z

however for the time being if you fix the merge conflict I guess we can merge it.

MarijnS95 · 2022-06-13T13:56:45Z

Ouch that's sad but very relatable. It works well enough for both our purposes (minus the few PRs I have open now) which I guess counts as "good enough" 😬. Thanks for bringing it this far though!

This in turn will require a lot of android apis for the android platform authenticator. android.security.identity and android.hardware.biometric, which is were I plan to fit in some experimentation with aidl.

That'd be very nice to combine then :D

however for the time being if you fix the merge conflict I guess we can merge it.

Will do, and will make a note to look into this because I'm curious to your --Map=filepath suggestion now 😁

MarijnS95 · 2022-06-13T14:02:12Z

Let's see if I resolved that correctly :)

Dynamic libraries that a Rust library links against while not being present on the target platform (`libc++_shared.so` and other custom libraries) need to be bundled in the APK specifically. This approach is adopted frm `android-ndk-rs` while switching from `readelf` to `llvm-readelf`.

MarijnS95 commented Apr 21, 2022

View reviewed changes

MarijnS95 force-pushed the readelf branch from 28549a7 to b001cc0 Compare April 25, 2022 14:42

MarijnS95 force-pushed the readelf branch from b001cc0 to d8e1692 Compare April 25, 2022 14:48

MarijnS95 commented Apr 25, 2022

View reviewed changes

xbuild/src/cargo/mod.rs Show resolved Hide resolved

MarijnS95 force-pushed the readelf branch 5 times, most recently from d7284c6 to be51030 Compare May 2, 2022 14:26

MarijnS95 force-pushed the readelf branch 4 times, most recently from e49abb9 to c09ada3 Compare May 4, 2022 08:51

MarijnS95 closed this May 4, 2022

MarijnS95 deleted the readelf branch May 4, 2022 09:02

MarijnS95 restored the readelf branch May 4, 2022 09:02

MarijnS95 reopened this May 4, 2022

MarijnS95 force-pushed the readelf branch from c09ada3 to 30b0849 Compare May 4, 2022 09:16

dvc94ch reviewed May 4, 2022

View reviewed changes

dvc94ch approved these changes May 4, 2022

View reviewed changes

MarijnS95 force-pushed the readelf branch from 30b0849 to 9e7066c Compare June 13, 2022 14:01

MarijnS95 force-pushed the readelf branch from 9e7066c to 4017930 Compare June 15, 2022 15:48

MarijnS95 force-pushed the readelf branch 2 times, most recently from 64e5ee2 to c5addcc Compare October 19, 2022 08:24

dvc94ch mentioned this pull request Dec 1, 2022

including libraries in a cross platform way #85

Open

MarijnS95 force-pushed the readelf branch from c5addcc to 823505e Compare December 1, 2022 14:52

dvc94ch approved these changes Dec 1, 2022

View reviewed changes

MarijnS95 and others added 3 commits December 1, 2022 17:41

llvm: Switch from llvm-readelf to llvm-readobj

c4b58ca

ci: Make llvm-readobj-13 available as llvm-readobj

2f4e91f

MarijnS95 force-pushed the readelf branch from 823505e to 2f4e91f Compare December 1, 2022 16:42

MarijnS95 merged commit a0d9b82 into rust-mobile:master Dec 2, 2022

MarijnS95 deleted the readelf branch December 2, 2022 09:01


		let ndk = env.android_ndk();

		let search_paths = []; // TODO: deps dir and everything from `cargo:rustc-link-search=`

Use llvm-readelf to detect and bundle needed dynamic libs for Android #65

Use llvm-readelf to detect and bundle needed dynamic libs for Android #65

Conversation

MarijnS95 commented Apr 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarijnS95 commented Apr 21, 2022

dvc94ch commented Apr 21, 2022

MarijnS95 commented Apr 25, 2022

MarijnS95 commented May 2, 2022

dvc94ch commented May 2, 2022 • edited Loading

dvc94ch commented May 2, 2022

MarijnS95 commented May 2, 2022

MarijnS95 commented May 4, 2022

dvc94ch commented May 4, 2022 • edited Loading

MarijnS95 commented May 4, 2022

MarijnS95 commented May 4, 2022

dvc94ch left a comment

Choose a reason for hiding this comment

dvc94ch May 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarijnS95 May 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dvc94ch May 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dvc94ch commented May 4, 2022 • edited Loading

MarijnS95 commented Jun 13, 2022

dvc94ch commented Jun 13, 2022

dvc94ch commented Jun 13, 2022

MarijnS95 commented Jun 13, 2022

MarijnS95 commented Jun 13, 2022

Use `llvm-readelf` to detect and bundle needed dynamic libs for Android #65

Use `llvm-readelf` to detect and bundle needed dynamic libs for Android #65

MarijnS95 commented Apr 21, 2022 •

edited

Loading

dvc94ch commented May 2, 2022 •

edited

Loading

dvc94ch commented May 4, 2022 •

edited

Loading

dvc94ch May 4, 2022 •

edited

Loading

MarijnS95 May 4, 2022 •

edited

Loading

dvc94ch May 4, 2022 •

edited

Loading

dvc94ch commented May 4, 2022 •

edited

Loading