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

set up free/play build flavors and move CastOptionsProvider to play #1206

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

eighthave
Copy link
Contributor

Description

The sets up the standard pattern for building free releases of apps that also use proprietary libraries. This draws on patterns used by Nextcloud, Element and other apps. There are two build flavors:

  • free: this should ultimately end up building a free open source APK.
  • gplay: this builds the current version, with all the existing proprietary dependencies.

This includes one fix as an example of how build flavors work for this kind of thing. The CastOptionsProvider comes from the proprietary Chromecast library. This moves all that to the gplay flavor.

first step towards #424

Testing Instructions

./gradlew assemble
./gradlew assembleFreeDebug
./gradlew assembleGplayDebug
  • Check whether Chromecast works in the gplay APK.

Checklist

  • If this is a user-facing change, I have added an entry in CHANGELOG.md
  • Ensure the linter passes (./gradlew spotlessApply to automatically apply formatting/linting)
  • I have considered whether it makes sense to add tests for my changes
  • All strings that need to be localized are in modules/services/localization/src/main/res/values/strings.xml
  • Any jetpack compose components I added or changed are covered by compose previews

@eighthave eighthave requested a review from a team as a code owner July 25, 2023 17:25
@CLAassistant
Copy link

CLAassistant commented Jul 25, 2023

CLA assistant check
All committers have signed the CLA.

@eighthave eighthave mentioned this pull request Jul 25, 2023
Copy link
Contributor

@mchowning mchowning left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution @eighthave!

1. Chromecasting is failing

Chromecasting isn't working for me on the gplay variant. Whenever I try to chromecast it gets stuck "Finding devices". Without the changes in this PR, I'm able to immediately find multiple chromecast devices. Is this working for you?

Screen.Recording.2023-07-31.at.3.20.54.PM.mov

2. CI is failing

CI is failing because it's trying to build without specifying the new dimension. I think updating the tasks here to be "['assembleGplayDebug', 'assembleGplayDebugAndroidTest']" will fix that.

3. [Nice to have] Automotive and Wear app modules and the free flavor

It doesn't have to be a part of this PR, but it seems like we should probably remove the free flavor from the automotive and wear app modules. I don't think it's possible to have a "free" automotive or wear os app (correct me if I'm wrong). What do you think?

base.gradle Show resolved Hide resolved
@eighthave eighthave force-pushed the build-flavors-free-gplay branch 2 times, most recently from 6b27756 to 0744c30 Compare August 4, 2023 18:30
@eighthave
Copy link
Contributor Author

  1. Chromecasting is failing

Oops, sorry about that. I used the wrong name for the flavor folder ("play" when it should have been "gplay". I don't have an easy way to test with Chromecast. So I used diffoscope to do a before/after diff on the APKs. I can see before the class and manifest entry were missing, now they are there.

  1. CI is failing

Should be fixed.

  1. [Nice to have] Automotive and Wear app modules and the free flavor

Sounds good to me. I worked with Gradle Build Flavors a fair amount, and I can't think of how to do this. Do you have a suggestion? Given the structure with base.gradle being shared across modules, I think it would require a bigger change to the Gradle setup.

@mchowning
Copy link
Contributor

mchowning commented Aug 4, 2023

Thanks for the updates!

For removing the build variants, I think something along these lines in the app and wear modules works to exclude the free variants.

android {

    //...

    variantFilter { variant ->
        if (variant.flavors*.name.contains("free")) {
            // Automotive app should not have a free variant
            setIgnore(true)
        }
    }
}

Unrelated to this, CI is still failing (but it's a new failure at least). I haven't found the fix yet, but it looks like the useGoogleServicesDebugFile task is not getting run anymore, and this is causing the build to fail due to the missing google-services.json file. If I execute ./gradlew :app:preBuild --info from the main branch, the useGoogleServicesDebugFile task runs, but it gets skipped with the changes on this PR (it gets skipped because "task onlyIf 'Task is enabled' is false"). 🤔

@eighthave
Copy link
Contributor Author

eighthave commented Aug 5, 2023

I'll try adding the code that removes the free tasks from wear and automotive.

I also saw that build failure intermittently on my machine. I can't actually see the build logs, it seems I need an Automattic account to see them on buildkite.com? Other CI services just let me click and see them.

I would solve it by removing that task, and just committing the google-services.json file in the debug flavor folder, e.g.:

git mv app/google-services.json_debug-only app/src/debug/

I don't understand what exactly that task is doing beyond just putting the file somewhere where the build will pick it up.

@eighthave
Copy link
Contributor Author

I'll be AFK for vacation, I'll come back to this when I'm back.

@eighthave
Copy link
Contributor Author

I'd like to continue to work on this. Any feedback on #1206 (comment) ?

@mchowning
Copy link
Contributor

mchowning commented Oct 2, 2023

I would solve it by removing that task, and just committing the google-services.json file in the debug flavor folder, e.g.:

git mv app/google-services.json_debug-only app/src/debug/

I don't understand what exactly that task is doing beyond just putting the file somewhere where the build will pick it up.

Seems like that should work—at least I can't think of any reason that wouldn't work now. It would be nice to simplify the build process. 👍

I'm assuming we won't need to update .configure since release builds would still fall back to the file at the root. We'll probably need to update .gitleaksignore though to ignore the new google-services.json files in the debug folders (and we can remove the currently ignored files if those get removed).

Since this change is directly dealing with how we handle secrets, let's make this change in a separate PR so it is easier to review and debug any issues that might crop up.

@eighthave
Copy link
Contributor Author

Since this change is directly dealing with how we handle secrets, let's make this change in a separate PR so it is easier to review and debug any issues that might crop up.

I posted #1434

eighthave added a commit to eighthave/pocket-casts-android that referenced this pull request Oct 6, 2023
It is not possible to have a it's possible to have a free software app work
on Android Automotive or Android Wear.  They must include proprietary libs
in order to work.

Automattic#1206 (comment)
@eighthave
Copy link
Contributor Author

eighthave commented Oct 6, 2023

I rebased this on #1434 and added #1206 (comment) to remove the free flavors from Automotive and Wear. The builds/tests pass locally

CastOptionsProvider comes from the proprietary Chromecast library.
It is not possible to have a it's possible to have a free software app work
on Android Automotive or Android Wear.  They must include proprietary libs
in order to work.

Automattic#1206 (comment)
@eighthave
Copy link
Contributor Author

I rebased this now that #1434 is merged, so this should be ready to go. Everything works locally for me. The one thing I couldn't figure out was how to use the new version catalogs syntax in the productFlavor block. The old syntax works there still.

@eighthave
Copy link
Contributor Author

Let me know when you're close to reviewing this and I can rebase and fix the merge conflicts.

@eighthave
Copy link
Contributor Author

I'm ready to pick this up again, just let me know when you can review it, and I'll rebase it on master.

@mchowning
Copy link
Contributor

I'm switching away from being a Pocket Casts core contributor, so someone else from @Automattic/pocket-casts-android will need to review this going forward.

@julianfoad
Copy link

Could someone else from @Automattic/pocket-casts-android review this? It looks like this was well on its way to being a useful step towards F-Droid eligibility #424 ...

@mchowning wrote:

I'm switching away from being a Pocket Casts core contributor, so someone else from @Automattic/pocket-casts-android will need to review this going forward.

@eighthave
Copy link
Contributor Author

If I know that it'll be reviewed, I'll happily fix the merge conflicts.

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.

None yet

4 participants