Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ndk-build,cargo-apk: Rename intent_filters back to intent_filter #305

Merged
merged 1 commit into from
Jul 5, 2022

Conversation

MarijnS95
Copy link
Member

@MarijnS95 MarijnS95 commented Jun 21, 2022

Fixes #304, CC @Hoodad

We commonly name vectors of items with their singular form so that they read nicer in TOML and XML (i.e. [[intent_filter]] adds a single entry to that list, and gets serialized as a single <intent-filter> XML element) and this matches Android's intent-filter naming too.

This change in a6f3e13 ("ndk-build: Move default serialization of MAIN intent filter to cargo-apk (#241)") was made purely with Vec in mind, but breaks existing manifest parsing nor was anticipated to be a breaking change in this area as the README still carries intent_filter instead of intent_filters in its Cargo.toml metadata reference.


I want to try landing this without yet-another breaking release for ndk-build: can we do that by yanking the previous releases? We don't need a breaking release for cargo-apk either way, but it may be nice to yank their older releases too.

@Hoodad
Copy link

Hoodad commented Jun 21, 2022

For what's its worth, I approve of the reasoning behind the name change. It aligns with, as you pointed out, Androids own way of doing it 👍

@MarijnS95
Copy link
Member Author

Behind keeping the name as it was, to be very precise 😬 - it's more in-line with all the other properties too.

Now let's see why NDK r23 is hitting us again - we already had to work around the libgcc bug and now it's breaking our CI :/

@MarijnS95
Copy link
Member Author

A relaunch seems to have fixed it, but we should perhaps bump the cache (newer api-level) at some point.

This is also the time ping @rust-windowing/publishers for suggestions/approval on breaking changes in a patch release if all the other versions are yanked; and I also need you to do the yanking if this is okay :)

@MarijnS95 MarijnS95 requested review from msiglreith and dvc94ch June 21, 2022 20:43
@MarijnS95 MarijnS95 force-pushed the back-out-of-intent_filters-rename branch from c3255c4 to 890a50a Compare July 4, 2022 18:37
We commonly name vectors of items with their singular form so that they
read nicer in TOML and XML (i.e. `[[intent_filter]]` adds a single entry
to that list, and gets serialized as a single `<intent-filter>` XML
element) and this matches Android's `intent-filter` naming too.

This change in a6f3e13 ("ndk-build: Move default serialization of `MAIN`
intent filter to `cargo-apk` (#241)") was made purely with `Vec` in
mind, but breaks existing manifest parsing nor was anticipated to be a
breaking change in this area as the README still carries `intent_filter`
instead of `intent_filters` in its `Cargo.toml` metadata reference.
@MarijnS95 MarijnS95 force-pushed the back-out-of-intent_filters-rename branch from 890a50a to 968c5dc Compare July 5, 2022 07:36
@MarijnS95
Copy link
Member Author

After some thought I still don't think it's a good idea to publish a breaking change in a patch release and yank the older versions, so we'll release it as breaking change instead.

@rust-windowing/publishers it'd still be nice to yank cargo-apk 0.9.0-0.9.2 and ndk-build 0.6.0 though, thanks!

@MarijnS95 MarijnS95 merged commit dc38fcb into master Jul 5, 2022
@MarijnS95 MarijnS95 deleted the back-out-of-intent_filters-rename branch July 5, 2022 08:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Custom intent broken since cargo-apk 0.9.0
3 participants