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

Flag optional Toga gradle dependencies. #1845

Merged
merged 6 commits into from
Jun 11, 2024

Conversation

freakboy3742
Copy link
Member

With the resolution of beeware/toga#2454, we're now in a position to soften the default Gradle dependencies for a Toga Android project. We can also include the known optional requirement for MapView .

PR Checklist:

  • All new features have been tested
  • All new features have been documented
  • I have read the CONTRIBUTING.md file
  • I will abide by the code of conduct

@freakboy3742 freakboy3742 requested a review from mhsmith May 28, 2024 03:05
@rmartin16
Copy link
Member

Works for me; although, I had to be careful of the version of Toga I was using. Should we consider bumping the Toga requirement in the bootstraps?

@freakboy3742
Copy link
Member Author

Works for me; although, I had to be careful of the version of Toga I was using. Should we consider bumping the Toga requirement in the bootstraps?

Strictly, that would only be needed for the Android backend; but regardless, I think it's more a case that we need to push out a new Toga release before this is merged. This change won't work until there's a public release that contains beeware/toga#2454; at which point, toga-android~=0.4.0 will resolve correctly, but toga-android~=0.4.0, >=0.4.5 will provide more safety.

@mhsmith
Copy link
Member

mhsmith commented Jun 3, 2024

For consistency this should also be done for the com.google.android.material library, which is only used by OptionContainer.

However, wouldn't it be better to include all of Toga's dependencies by default? Before assuming any of them are too large, we should actually measure their effect on the size of the app.

@freakboy3742
Copy link
Member Author

For consistency this should also be done for the com.google.android.material library, which is only used by OptionContainer.

I don't think that's entirely true - we're using Material as a proxy for installing androidx.ConstraintLayout, plus a couple of other dependencies that are fairly fundamental to app operation.

It looks like we could also be using it as a proxy for androidx.appCompat, as that package is a dependency of Material.

However, wouldn't it be better to include all of Toga's dependencies by default? Before assuming any of them are too large, we should actually measure their effect on the size of the app.

I've just built a HelloWorld Android app with 3 different gradle configurations; the AAB sizes for those apps are:

  • Including material, appCompat: 24160823
  • Including material, appCompat, SwipeRefreshLayout and OSMDroid: 24624641
  • Including material only: 24160759

So - there's about a 500kb difference in size including packages that aren't strictly needed. Explicitly including a dependency that is implied has a 100b cost, but that could easily be a quirk of compression.

500kb isn't much, but it also isn't nothing, so my inclination is to keep them optional (and lean into the fact that Material implies AppCompat).

@freakboy3742
Copy link
Member Author

I've also pushed the lower version pin for Toga; this will need to wait until 0.4.5 has landed before CI will pass.

Copy link
Member

@mhsmith mhsmith left a comment

Choose a reason for hiding this comment

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

500kb isn't much, but it also isn't nothing, so my inclination is to keep them optional

OK, I guess we'll find out soon enough if people have trouble with it.

@@ -51,15 +51,15 @@ def pyproject_table_macOS(self):
return """\
universal_build = true
requires = [
"toga-cocoa~=0.4.0",
"toga-cocoa~=0.4.0, >=0.4.5",
Copy link
Member

Choose a reason for hiding this comment

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

This can be simplified to ~=0.4.5.

@freakboy3742
Copy link
Member Author

Toga 0.4.5 has been released; I've merged with main, and the tests now pass.

@mhsmith mhsmith merged commit ed7e596 into beeware:main Jun 11, 2024
52 checks passed
@freakboy3742 freakboy3742 deleted the toga-android-gradle-deps branch June 11, 2024 21:42
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