Added support for adding extra files to the APK. #141
Conversation
| # Path to a folder containing a hierarchy of extra files to be added at the root of the APK. | ||
| # All files will be added to the APK recursively. | ||
| # If not specified, no extra files will be added to the APK | ||
| extra_content = "path/to/extra_content_folder" |
There was a problem hiding this comment.
I guess assets/resources are intended to be relative paths included in version control, while in your use case it's probably an os specific path. Also this doesn't help select the right library for the target architecture. Are there additional use cases for extra-content?
There was a problem hiding this comment.
Are you suggesting that I write the example as an absolute path?
In my particular use-case I do have the shared library file under my repository directory (although not included in version control), so I'm using a relative path.
Regarding selection of the right library for the target architecture, if the APK contains multiple architecture, I think what happens is that dlopen() already appropriately uses the correct set of paths (including the proper lib subfolders in the APK) depending on which architecture is used when running the app.
As for additional use cases, honestly, I can't think of any off the top of my head.
There was a problem hiding this comment.
I don't think we produce fat apk's because that can lead to very large apks. We could add the feature.
There was a problem hiding this comment.
@dvc94ch Apparently cargo-apk already produces fat apks when more target architectures are specified in build_targets. I don't know if separate APKs can be distributed anyway unless we dive into the new "app bundles" system.
Regarding assets/resources we have to be very careful that those still travel the normal aapt package -A/-S as they need additional processing and are not "zipped" into the target APK verbatim.
| fn add_entry(&self, base_path: &Path, path: &Path) -> Result<(), NdkError> { | ||
| let full_path = base_path.join(path); | ||
| if full_path.is_dir() { | ||
| for entry in fs::read_dir(full_path)? { |
There was a problem hiding this comment.
Can't we pass a directory to aapt add so that it adds them recursively?
If not you're better off building an array of filenames and adding them to the archive in one go, spawning an aapt add process per file can potentially become very costly and wasteful.
| std::fs::copy(base_path.join(path), out)?; | ||
|
|
||
| let mut aapt = self.0.build_tool(bin!("aapt"))?; | ||
| aapt.arg("add").arg(self.0.unaligned_apk()).arg(path); |
There was a problem hiding this comment.
Should we make sure that aapt runs relative to the build directory here? There are environment variables dealing with moved target/ directories and I don't exactly remember how they are handled here, if we keep running in the right PWD.
If we do that this might as well run aapt with base_path as PWD.
| } else if full_path.is_file() { | ||
| self.add_file(base_path, path)?; | ||
| } |
There was a problem hiding this comment.
May need an error if it's not a directory nor a file (ie. a special file like a character device) just in case?
|
|
||
| pub fn add_content_dir_recursively(&self, content_dir: &Path) -> Result<(), NdkError> { | ||
| if !content_dir.is_dir() { | ||
| return Err(NdkError::PathInvalid(content_dir.to_path_buf())); |
There was a problem hiding this comment.
I'd say something along the lines of "Is not a directory". That way the path can either not exist at all, or be a file. Invalid is a tad too generic IMO.
|
One thing to note while seeing #142 pop up just a couple hours ago: a generic codepath might not be desired for libraries. |
|
#142 looks good to me, I'll test it against my use-case, but it's probably a better solutions for various reasons:
|
|
@agrande @kanerogers then I prefer #138 which was here first and takes care of the |
|
Ok, then, I'll re-open and modify #138 to use |
Ah, that's a very good point. I hadn't thought of that! |
This replaces #138 as a more generic solution.