Manifest (de)serialization

[maxded]

This PR contains major changes to the Manifest struct in ndk-build, changing it to a proper (de)serialization struct. The Cargo.toml's [package.metadata.android] is now directly deserialized into Manifest and can be serialized into an AndroidManifest.xml using quick-xml.

The AndroidManifest structure matches, android's manifest structure and it's components. Note that not all components have been implemented yet, just the ones that were already implemented.

One downside of my current implementation is the verbosity in the .toml file. Having to write [[package.metadata.android.application.activity.intent_filter]] is a bit much. If needed this could be improved by implementing Deserialize manually (I think, never implemented Deserialize myself yet).

All default values remain the same so for a user of cargo-apk, the only thing that changes is the syntax in their Cargo.toml, the generated AndroidManifest is exactly the same.

Hope this is a welcome change :)

[msiglreith]

@MarijnS95 would you be fine with the proposed changes as well?

[MarijnS95]

@msiglreith Yeah I'm totally content with the proposed refactor, which I suggested to do after wading through the "pipeline" (and stringformatted xml 🙈). This is still stuck in my review queue, will try to prioritize it first thing tomorrow morning :)

EDIT: This is now lifted into the weekend, got a bit distracted with other android-ndk issues and PRs, apologies!

[msiglreith]

In general I'm fine with the new manifest structure, thanks for that!
I would prefer to keep some of the defaults tho to better match the old manifest. In particular, it should be possible to run most applications using bare minimum changes to ones cargo toml. As example, the current ndk-examples won't execute on my phone due to missing MAIN action intent.

[maxded]

All the default values from the previous implementation should be present in my implementation except for

• activity: label
• activity: launchMode
• activity: screenOrientation

Android provides default values for these components when missing from the manifest, which are the exact same as the previous default values so I left them out, should we be explicit here or is that ok?

[msiglreith]

Android provides default values for these components when missing from the manifest, which are the exact same as the previous default values so I left them out, should we be explicit here or is that ok?

Thanks, should be good!

[msiglreith]

I would go forward with merging it tomorrow if there are no reasons against doing so

[MarijnS95]

Sorry for holding this up so long. I'll go through the changes now and let you know.

[MarijnS95]

Looking really good thus far! This review contains a couple questions and suggestions to simplify the code. Things to pay attention to:

• Android requires certain fields to have a specific value. Some are filled with defaults upon deserialization, others are unconditionally overwritten during build. That should be streamlined and made more explicit to the user;
• Tests: While the readme is nice I'd suggest moving it over to an example app that is actively parsed and tested by the CI. That way the documentation can hardly ever get out of sync with the actual parsing logic/structure in Rust.

[MarijnS95]

That cleans up really nicely! Some nits on the last commit, will check the remaining comments after.

[maxded]

@MarijnS95 think we are good to go now!

[MarijnS95]

@maxded Thanks! We're almost there, just a couple more documentation changes. Be sure to check the unresolved comments above, we should either fix/discuss those here or open an issue to follow up on them.

[maxded]

@MarijnS95 resolved all the comments above!

Be sure to check the unresolved comments above, we should either fix/discuss those here or open an issue to follow up on them.

What exactly did you mean here? Running cargo test --doc in CI or?

[MarijnS95]

Perhaps worth mentioning that this is a bitmask with the same encoding as Vulkan version numbers, representing the value of VkPhysicalDeviceProperties::apiVersion (and has nothing to do with Vulkan features in the sense as the other two features).

[MarijnS95]

What exactly did you mean here? Running cargo test --doc in CI or?

The question where to perform manifest default overrides: #106 (comment)

Testing all docs besides (target-specific) ndk docs was a separate bulletpoint in the "PR review description" apparently; regardless we should look into this.

[maxded]

Moved the non-artifact dependent default overrides to the from_subcommand function as, like you mentioned, that makes more sense indeed.

[MarijnS95]

Moved the non-artifact dependent default overrides to the from_subcommand function as, like you mentioned, that makes more sense indeed.

Great, thanks! Upon second thought it might have been better situated in fn build() (including the parse_from_toml call, cmd is available in ApkBuilder anyway) since no other call needs it, even though they all call fn build() now anyway. Bit unfortunate that we still need the clone and mut to set one or two extra fields basted on the Artifact to build.

Anyway, I guess this looks good now! Haven't given the final code a full read-through but the last diff addresses the remaining issue. Thanks for pulling it all the way through 🥳 !

@msiglreith Do you want to give this a quick look and make sure I didn't miss anything? Then how do we want to release this, just bump the version and throw it out in the wild, hoping all projects read the changelog and update their toml?

[msiglreith]

Really nice and thanks for the review! A few minor nitpicks from me (:

Then how do we want to release this, just bump the version and throw it out in the wild, hoping all projects read the changelog and update their toml?

Mostly, yeah 😄 With the current CHANGELOG and example manifest it should be pretty clear on how to transition imo.

[maxded]

Done!

[MarijnS95]

Fix cargo doc

Not specifically the docs, but the rustdoc run disallows any warnings in documented code (/// ```rust stuff) and apparently also any code outside it! The test run (https://github.com/rust-windowing/android-ndk-rs/runs/2168935502) finds these issues as well but doesn't exit with a failure code. We'll need to add some explicit check/clippy runs to the CI in the future...

[MarijnS95]

See the README for...?

[MarijnS95]

This all looks okay to me. I guess we're just too afraid to press the merge button on larger changesets and do a release, but we should push through anyhow. Let's make this happen and give it some wider attention 🎉