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

Release 0.5.1 #149

Merged
merged 2 commits into from
Dec 20, 2023
Merged

Release 0.5.1 #149

merged 2 commits into from
Dec 20, 2023

Conversation

rib
Copy link
Collaborator

@rib rib commented Dec 20, 2023

@@ -6,6 +6,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

## [Unreleased]

### Changed
- Avoids depending on default features for `ndk` crate to avoid pulling in any `raw-window-handle` dependencies ([#142](https://github.com/rust-mobile/android-activity/pull/142))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a breaking change fwiw... But we might just let it slide?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As long as we add something along the lines of Warning: if you are depending on the `rwh_06` feature from the `ndk`, enable it via a manual dependency of sorts.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I had overlooked that we expose the NativeWindow type.

I'm not 100% sure atm. It's a bit of a fuzzy area.

In this guide for Rust semver it would generally consider changing a feature for a dependency a minor change if that doesn't introduce a breaking change: https://doc.rust-lang.org/cargo/reference/semver.html#cargo-change-dep-feature

The question would be whether to consider the NativeWindow API change to be a breaking change for the android-activity API - and I'm not really sure that it does.

In practice, probably all that matters is whether Winit, or other crates that use raw-window-handle API are ensuring they enable whatever version they need explicitly without somehow being dependent on the default features that android-activity might enable.

In winit it looks like rwh_06 is also a default that explicitly enables the corresponding ndk feature so I don't think anything based on Winit will be dependent on whatever default android-activity enables.

So I think it might be safe to not consider this a breaking change?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if that doesn't introduce a breaking change

It does, because an expected trait on a public type is no longer implemented, causing code depending on this to fail to compile.

In practice, probably all that matters is whether Winit, or other crates that use raw-window-handle API are ensuring they enable whatever version they need explicitly without somehow being dependent on the default features that android-activity might enable.

Winit may use this (and turns on the appropriate features), but it's more likely that a user is also utilizing it and not one of our plumbing crates.


We should always consider this a breaking change. The question is, are we okay with cheating and pretending it is safe for a patch release?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a grey area because:

  1. Probably barely anyone is using this raw;
  2. It's easy to turn on the feature after all (but it's still a compilation-breaking change).

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm admittedly quite biased because I want this shipped, so just acknowledging that upfront.

I agree with @MarijnS95 that this is a breaking change, but that it still falls into a gray area because of the relatively minor consequences. This isn't asking those affected, if there are any, to commit significant effort to accommodate us. It's asking them to update their project with an explicit feature flag. We should strive to make this need well documented so that, if someone's build process breaks, that the time loss is minimal and the rationale behind the change is clear.

IMO, all software development processes exist to serve us and to make life easier on us. In scenarios where we feel beholden to the process, but our intuition hints that the process might be limiting us, we should favor our intuition rather than becoming encumbered.

If I were depending on this project, and this update broke my project, but I were able to quickly go to the changelog and find exactly what happened with supporting discussion, I would not significantly fault anyone here. There would be a minor annoyance that would then pass quickly as I discover the fix is trivial.

That said, it's not trust in my project that's on the line here and without stake in the game I don't know how much weight my opinion should hold.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep; I also see semver as a tool, and there can be cases where for all intents and purposes a change that is technically a semver break isn't actually going to be an API break in practice - and this seems to be one of those cases.

Playing devils advocate briefly though - I have to admit I also don't really see that this is that important of a change for justifying potential breakage (even though breakages seems fairly unlikely) and maybe Bevy shouldn't see it as a blocker that for an interim period it may pull in two versions of raw-window-handle while it's stuck using 0.5.

The ideal fix at some point will be that the ndk crate won't pull in any raw-window-handle crate by default - and by the same logic used here, couldn't we also just remove the default from the ndk crate which would be more beneficial I'd guess?

To some extent android-activity is essentially just one arbitrary crate that happens to depend on the ndk crate with default features, and instead of having a whack 'a' mole situation where you'd need to stop all crates depending on the ndk from using default features if you're trying to avoid raw-window-handles 0.6 then we could instead address this in the ndk crate.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rib I've already confirmed that this will be gone from the next breaking ndk release; but I find it more likely that those with raw use of the ndk crate rely on its raw-window-handle integration than those depending on it only indirectly via the android-activity crate.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As long as we add something along the lines of Warning: if you are depending on the `rwh_06` feature from the `ndk`, enable it via a manual dependency of sorts.

yeah, I've put in a big blurb explaining that technically the change is breaking and said to enable rwh_06 explicitly in the unlikely case that something was depending on it being enable by default by the android-activity crate's ndk dependency.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rib I've already confirmed that this will be gone from the next breaking ndk release; but I find it more likely that those with raw use of the ndk crate rely on its raw-window-handle integration than those depending on it only indirectly via the android-activity crate.

Right, I'm wondering, out-loud, though if we could also justify removing the default for the ndk crate in a patch release too if we also figure that it's unlikely that anyone depends on it currently.

Although this crate has some examples that depend on the ndk, they
aren't regular Cargo examples, they are completely standalone apps
that depend on dev-dependencies.
android-activity/CHANGELOG.md Outdated Show resolved Hide resolved
android-activity/CHANGELOG.md Outdated Show resolved Hide resolved
android-activity/CHANGELOG.md Outdated Show resolved Hide resolved
android-activity/CHANGELOG.md Outdated Show resolved Hide resolved
@rib rib merged commit 967882f into main Dec 20, 2023
6 checks passed
@rib rib deleted the release-0.5.1 branch December 20, 2023 22:11
@rib rib mentioned this pull request Dec 20, 2023
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.

3 participants