Add support for dynamically loaded libraries

[agrande]

Sometimes we need to add .so libraries to the apk that are not linked against the binary, with the intent of loading them dynamically with dlopen().

This commit allows to do that by placing those libraries at the root of the local package, in a lib/${androideabi} folder.

The build command now scans for those folders and adds the found .so files to the apk with aapt.

[agrande]

(My use case for this is loading Oculus's implementation of OpenXR on Oculus Quest)

[MarijnS95]

Thanks! That looks simple enough!

Don't forget to add this feature to the relevant cargo-apk/ndk-build changelogs, please :)

[agrande]

Ok, done the requested changes.

[dvc94ch]

Looks ok. Although maybe you want a custom build.rs that uses cargo-apk as a library instead?

[MarijnS95]

Although maybe you want a custom build.rs that uses cargo-apk as a library instead?

@dvc94ch What exactly do you mean with this? build.rs emits link paths and libraries that are to be linked into the library, either static or dynamic - not at runtime using dlopen. I don't think we can leverage that.

[dvc94ch]

but you could add an additional library to the apk, we even have an api for it https://github.com/rust-windowing/android-ndk-rs/blob/master/ndk-build/src/apk.rs#L136

[agrande]

Looks ok. Although maybe you want a custom build.rs that uses cargo-apk as a library instead?

It doesn't look possible to do this with the public API currently exposed by cargo-apk. I would need a hook to perform operations on the unaligned apk to make it possible. But maybe I'm missing something?

[MarijnS95]

we even have an api for it https://github.com/rust-windowing/android-ndk-rs/blob/master/ndk-build/src/apk.rs#L136

@dvc94ch That is exactly the function called by this PR from fn add_extra_libs, no? It's just about finding them in lib/<abi> in the first place.

The current caller of add_lib only concerns the dependency chain of dynamically linked libs, not runtime linked ones.

[MarijnS95]

@dvc94ch Ah, you want to call ndk-build from a build.rs script and use add_lib from there? Sounds cumbersome as all the other code around it would need to be replicated too.

However: it might be a good idea to make this behaviour configurable. lib/ is quite a common name/path, and I don't think we want to add user files into their APKs without them knowing (despite this being printed clearly at the end of the log).

If we add this path as an optional configuration option to Cargo.toml that's solved.

[dvc94ch]

my point is that someone has to manually copy them in to the libs folder which isn't a cargo recognized folder as far as I know. So maybe setting an env variable OCULUS_SDK and using a build.rs would be more convenient (my distro wouldn't install it in a cargo target folder anyway). I do see that the current ApkBuilder exposed by cargo-apk isn't very convenient for this use case, but maybe we can add a constructor that doesn't require cargo-subcommand to work? I'd certainly favor making the cargo-apk api more user friendly than adding "hacks". But I'm also not opposed to this PR.

[agrande]

However: it might be a good idea to make this behaviour configurable. lib/ is quite a common name/path, and I don't think we want to add user files into their APKs without them knowing (despite this being printed clearly at the end of the log).

If we add this path as an optional configuration option to Cargo.toml that's solved.

That's what I originally intended to do, but one problem I see with that is that the aapt command used by add_lib doesn't allow specifying an input directory that is different from the target directory. So we'd need to cwd to the configured path to make it possible (as dlopen() will only look for shared libraries in the lib folder of the apk).

[MarijnS95]

my distro wouldn't install it in a cargo target folder anyway

I don't think it'd install them in any easily accessible location anyway, assuming you're running the SDK and development on an x86 PC and the libraries likely available for the common mobile processors (arm and aarch64).

That's what I originally intended to do, but one problem I see with that is that the aapt command used by add_lib doesn't allow specifying an input directory that is different from the target directory. So we'd need to cwd to the configured path to make it possible (as dlopen() will only look for shared libraries in the lib folder of the apk).

add_lib copies the file to a temporary directory that adheres to lib/<abi>/libsomething.so anyway, so that shouldn't be a problem?

@dvc94ch has a good point: it'd be nice if we could support system locations as well, ie. extra_libs = ["/opt/oculus_sdk/something/prebuilts/android/.../*", "some/other/glob/*"] but we'd have to work out how to support multiple ABIs this way.

In the end we might want to settle on a more generic way to include extra files, as we already do for resources and assets.

[agrande]

my point is that someone has to manually copy them in to the libs folder which isn't a cargo recognized folder as far as I know. So maybe setting an env variable OCULUS_SDK and using a build.rs would be more convenient (my distro wouldn't install it in a cargo target folder anyway). I do see that the current ApkBuilder exposed by cargo-apk isn't very convenient for this use case, but maybe we can add a constructor that doesn't require cargo-subcommand to work? I'd certainly favor making the cargo-apk api more user friendly than adding "hacks". But I'm also not opposed to this PR.

For my specific use-case there would be a manual step in any case, as I don't think I can freely distribute the shared library (as part of a published crate or otherwise) anyway. It has to be manually downloaded from the Oculus Developper website as far as I know.

I understand that loading dynamic libraries at runtime may not be a common enough use-case to justify direct support in cargo-apk, so I'm open to discussing the alternative (providing a finer-grained API).

[dvc94ch]

it'd be nice if we could support system locations as well, ie. extra_libs = ["/opt/oculus_sdk/something/prebuilts/android/.../", "some/other/glob/"] but we'd have to work out how to support multiple ABIs this way.

well that won't work since it's os/distro specific. that's why I suggested a build.rs that uses an env variable to find the right .so for the architecture.

I understand that loading dynamic libraries at runtime may not be a common enough use-case to justify direct support in cargo-apk, so I'm open to discussing the alternative (providing a finer-grained API).

It's not that I think it's not common enough, but that I think it would be a better solution as manually keeping stuff in sync is annoying. Also as a (not really) maintainer I want to keep the codebase as small as possible.

[MarijnS95]

@agrande FWIW APKs are just ZIPs, it doesn't seem to bad to me to just have some option in Cargo.toml to point to a directory (or multiple) whose contents (ie. a lib/<abi>/* kind of structure) as a whole are zipped up into the APK. That way this is both extensible and generic enough for whatever usecase one might need.

[agrande]

add_lib copies the file to a temporary directory that adheres to lib/<abi>/libsomething.so anyway, so that shouldn't be a problem?

Oh, right, I didn't see it before, my bad.

In the end we might want to settle on a more generic way to include extra files, as we already do for resources and assets.

Agreed with that.

[MarijnS95]

well that won't work since it's os/distro specific. that's why I suggested a build.rs that uses an env variable

@dvc94ch Exactly, it's just an example and we might want to expand globs, walk symlinks and resolve environment variables to make it so.

to find the right .so for the architecture.

APKs support multiple architectures so it shouldn't be a problem to add them all? It is of course cleaner to use just the architectures listed in build_targets, but alas.

EDIT: I don't remember if we actually generate a unique APK per architecture instead of one that contains libraries for all targets.

[dvc94ch]

Exactly, it's just an example and we might want to expand globs, walk symlinks and resolve environment variables to make it so.

Let's please not add a dsl for this use-case. I don't understand why a build.rs isn't ok. I guess one problem is that the build.rs would have to be in the binary. But you could have an oculus-sdk-build crate then people using the oculus sdk would only have to add oculus_sdk_build::build() to their build.rs

[dvc94ch]

I guess I'm a little biased. cargo-apk and ndk-build were ment to be libraries so that you could build more sophisticated things like cargo-flutter for example. But cargo-flutter died anyway, so maybe there are not that many usecases left for a more flexible api.

[MarijnS95]

Let's please not add a dsl for this use-case.

Hehe of course not, we have to settle on something that is common and generic.

I don't understand why a build.rs isn't ok.

The only reason is that I don't see an example of how to use this from build.rs. My fear is that it would be too complex, ie. having to copy-paste bits of the code from cargo-apk and manually run through all the build steps, when all @agrande wants is to add some arbitrary files to the final APK.

[MarijnS95]

I guess I'm a little biased. cargo-apk and ndk-build were ment to be libraries so that you could build more sophisticated things like cargo-flutter for example. But cargo-flutter died anyway, so maybe there are not that many usecases left for a more flexible api.

I'm pretty sad that for example cargo-ndk exists this way instead of leveraging ndk-build. Do more with less, and it's better to get development focused on a single crate performing this task instead of two separate/competing implementations doing roughly the same thing.

Perhaps we should just add and release a thing like that and see how it goes, as mentioned in the other issue.

[dvc94ch]

Let's not forget that cargo-ndk happened before ndk-build existed ;) the old cargo-apk and cargo-ndk were inspiration for this cargo-apk

[agrande]

The only reason is that I don't see an example of how to use this from build.rs. My fear is that it would be too complex, ie. having to copy-paste bits of the code from cargo-apk and manually run through all the build steps, when all @agrande wants is to add some arbitrary files to the final APK.

That sums it up pretty well for me.

@agrande FWIW APKs are just ZIPs, it doesn't seem to bad to me to just have some option in Cargo.toml to point to a directory (or multiple) whose contents (ie. a lib//* kind of structure) as a whole are zipped up into the APK. That way this is both extensible and generic enough for whatever usecase one might need.

I think this is the best way to handle my use-case without being too specific.
Maybe I should close this PR and open a distinct one for that?

[agrande]

Made a more generic PR: #141

[MarijnS95]

@agrande Yup it looks like you have about everything now. We ended up merging #106 as that was already scheduled around the weekend, and has some overlap in the struct changes you did here. Apologies for the conflicts, but they should be easier to solve here than to conflate and delay that significant change even further. I hope that's okay for you to resolve.

I don't think you need to store runtime_libs in struct Config though, given that it's only ever used in the same fn build() a couple lines below, that should save on some struct churn and propagating the member.

[MarijnS95]

Furthermore I briefly mentioned potential slowness/overhead in repeatedly calling aapt add for every file in #141 (comment). Not something to resolve now, but perhaps we should refactor this code a bit in a future MR to keep track of a list of libraries that need to be included - and have hence already been scanned and recursively walked by readelf. In the end we can pass that list to aapt add at once so that it should be able to efficiently package multiple files up in the target zip, without repeatedly spawning processes, reopening the zip file, shuffling data around and overwriting file tables.

[agrande]

@MarijnS95 no problem, I'll resolve the conflicts.
As for grouping the calls to aapt in one, indeed that would be a significant optim, but yeah, I think it would be better in a separate PR as it has a larger impact than what this PR does.

[msiglreith]

an error message would be good if this condition fails

[agrande]

Just printing a warning, or do you think I should return an error (such as NdkError::PathNotFound)?
That would stop the process. I'm not sure that's what we want here.

[MarijnS95]

It definitely needs to be an error since the user explicitly configures this path and should be told when it doesn't exist instead of silently going unnoticed (ie. typo, capitalization mismatch in the name).

You don't really need to do anything though - just remove if abi_dir.is_dir() and let fs::read_dir(abi_dir)? boil up the error if the path is invalid.

Though OTOH io::Error doesn't include the path leading to usual generic "file not found" message without any context. We should add a new error message expanding Io(#[from] IoError) with something like IoPath(#[source] IoError, String) and .map_err(|io| IoError(io, path), but that's something for a different PR and to be applied globally! I can take care of this later unless you feel like adding it.

[agrande]

I removed if abi_dir.is_dir() for now.
I don't mind making a PR for the error mapping at a later time if needed.

[MarijnS95]

In hindsight I don't see much if any applicable error unwraps (comment out IoError and follow all compiler errors for missing From implementations), feel free to add this for just this case in this PR.

[agrande]

I'm not sure exactly how to do that, to be honest (still very much new to Rust), but I'll look into it.

[MarijnS95]

It's a fairly trivial thing to do. Instead of explaining I pushed another commit to this PR showing how :)

The error string isn't all too pretty but it's hard to provide a proper string when we have no idea what error occurred; just that we know it happened while interacting with the specified path.

[agrande]

Thanks! 😀

[MarijnS95]

Yw! Any suggestions for a better string name though? And we could apply this to a few other locations but I'll reconsider where in a future PR.

[MarijnS95]

Cool!

I think we're done here now? Nice little change to get in 🙂

[zmerp]

@dvc94ch @msiglreith Ping 🙂 This is blocking progress on openxrs and bevy.