Fix add_lib_recursively duping deps across needed and runtime libs

[zer0def]

This is a fix to an unfortunate regression when iterating on #187, as every add_lib_recursively call tracks provided libraries separately, leading to situations where chain-dependencies found by readelf and in runtime_libs cause aapt to fail due to attempting the same library twice.

[MarijnS95]

Like I said in a previous review, why don't we postpone calling add_lib until after this provided HashSet has been set up? That way we can get away with calling aapt add with all files at once, and would have probably saved #186 from regressing at least twice?

[dvc94ch]

should we revert the commits for now?

[MarijnS95]

@dvc94ch It looks like my comment has sort of been addressed, maybe this is ready for merge now but I typically hold off reviewing until at the very least all the basic checks in the CI succeed.

Yes, I do feel more comfortable reverting it all, then reapplying+squashing it into a single fresh PR (could be this one) so that we can re-review this feature from the ground up.

On the other hand I don't think we have a lot of stuff that we'd like to release now; I wanted to have [env] support for vendored NDK but then you xbuild tool came in clutch!

[zer0def]

Sorry for running late on this, updated per linter recommendations.

[MarijnS95]

Unless I've been misunderstood it seems the call to add_lib has now not been removed/postponed out of add_lib_recursively. Not that it wouldn't work OOTB since the provided list also contains libraries that are already available on the phone and should not be copied.

This code is getting pretty messy and horrible (mutating state in RefCells don't exactly make it easier to read), in xbuild I opted for a simpler approach where add_lib_recursively becomes list_needed_libs_recursively which returns a HashSet, that can simply be recombined with results of the other list_needed_libs_recursively calls for the runtime libraries.

Then we only need to figure out what to do when paths to libraries differ, but their name ends up being the same. Could be worth an error, but shouldn't happen if the search paths remain the same across list_needed_libs_recursively calls unless the user places one of the needed libraries in the runtime_libs directory as well.

[zer0def]

I agree with the sentiment entirely, as having two sources that can potentially overlap deeper in their dependency hell isn't doing the current impl's any favors. That would require a much more significant refactor than I'm currently capable of making in a timely manner, so it's better to revert previous iterations of this feature.

[MarijnS95]

Can't we just pass &mut self here and get rid of the RefCell? Then it's just a matter of splitting UnalignedApk in two (apologies, I can't figure out proper naming for these two right now) where one carries a HashSet of libraries to be bundled (provided but without the stuff from android_search_paths, including libc++_shared.so where applicable), and a function that "converts" to the other type by means of zipping/adding the libraries in this HashSet (perhaps other files later on, if necessary) at once? Should be more efficient too, and doesn't sound too complicated.

[MarijnS95]

I do realize now that the edge-case for libc++_shared.so is more messy than I thought, with the if - else if making things really hard to follow, and #239 to fix that by duplicating the name again. Still thinking on how to simplify that, for xbuild too.