Request: a build flavor without GMS

Question

Request: a build flavor without GMS

pazos opened this issue 4 years ago · 20 comments

Coolreader updates on F-droid are blocked caused by the recent inclusion of Google services. If I understand correctly they were added to support GDrive sync.

I would like to solve this but I'm not sure if upstream is wishing to spend time on reviewing this. Ideally it would be
a couple of folders under android/app/src. Lets call them gplay and fdroid. Within each folder a new class with the same package name and same class name.

That class would be the one that implements methods that rely on GMS within the program. This for the gplay flavor. The Fdroid flavor would include empty methods. Both classes should contain a boolean method called something like isSupported which returns true on the former and false on the later.

The changes needed on the common code would be checking that method response and invoke the specific method needed for a functionality. Something like:

if (gms.isSupported()) {
    gms.doSomething(params)
}

And include gms as a depency only on gplayimplementation

The other alternative would be to fork the project and remove GMS in the fork. That would be easier to implement but harder to keep updated. Pinging @virxkane

Answer 1 · 2021-05-09T16:24:37.000Z

F-droid version is definitely needed
@pazos by the way, some other book reader app has Google Drive sync in its F-Droid version.

Answer 2 · 2021-05-09T17:29:28.000Z

@warren-ru: I'm fairly sure there's no problem with an app that syncs with Google Drive if the source code that handles the sync is open sourced. The app would be flagged no free net or something like that.

That's not the case here, where the commit f7ff381#diff-9526ccfd1d1813ed49c39f8c54dbeb512607376a007d824b905bc8b4e4d202d9 introduced a dependency on a closed source blob.

Answer 3 · 2021-05-10T19:50:07.000Z

I would like to solve this but I'm not sure if upstream is wishing to spend time on reviewing this.

I have nothing against it. The main thing is not to remove functionality from the main version.

if (gms.isSupported()) {
    gms.doSomething(params)
}

May be gms.isImplemented() sounds better?

I'm fairly sure there's no problem with an app that syncs with Google Drive if the source code that handles the sync is open sourced.

What are the open source libraries for working with Google Drive for Android?

Answer 4 · 2021-05-10T20:20:22.000Z

Also, I would not like to scatter the source codes in directories with too different paths, overly complicate the project structure. The main sources of the fronted android are now "android/src". You propose to transfer the sources of the Google Drive sync module to <android/app/src/gplay>, to be honest, I would not want to split the source tree like that.

And instead of gplay better name is gdrive.
Perhaps you could think of something more elegant, something like checking BuildConfig.BUILD_TYPE. I'm not sure if we need BUILD_CONFIG exactly, but maybe there is something even more suitable.

Just for reference: https://developer.android.com/studio/build/build-variants

Answer 5 · 2021-05-10T20:49:00.000Z

Thanks for your time @virxkane

May be gms.isImplemented() sounds better?

Totally cool, as you wish!

What are the open source libraries for working with Google Drive for Android?

No idea. I was talking about suppositions :). Maybe https://rclone.org/ can be used in android using a JNI wrapper.

Also, I would not like to scatter the source codes in directories with too different paths, overly complicate the project structure. The main sources of the fronted android are now "android/src". You propose to transfer the sources of the Google Drive sync module to <android/app/src/gplay>, to be honest, I would not want to split the source tree like that.

In general I agree with you but in this case is not possible. I will try to explain:

If the feature is self-contained then you can implement it without problems and run it on the flavors you want (like checking BuildConfig.HAS_SOME_FEATURE)

For features that rely on 3rd party libs this won't work. If the class is generic and relies on google services it will fail to build the flavor where these deps weren't defined.

It is totally possible to have the main sources in current path and just override the class that implements the sync. I didn't dig in the code but after a quick look I think the Gdrive sync was implemented in place. If all these methods were implemented on a single class (for example android/src/org/coolreader/sync2/Gdrive.java) it should be possible to leave the class untouched and just add a dummy class android/fdroid/org/coolreader/sync2/Gdrive.java. It is also possible to simplify the new folder to android/fdroid/Gdrive.java but Android Studio will complain that the folder doesn't match the package name.

And instead of gplay better name is gdrive

Actually, following the previous example there's no need to have a flavor for gplay/gdrive. It should be part of the main flavor (the only one now).

Perhaps you could think of something more elegant, something like checking BuildConfig.BUILD_TYPE. I'm not sure if we need BUILD_CONFIG exactly, but maybe there is something even more suitable.

Sure. A BuildConfig property will work. We use that in KOReader (see https://github.com/koreader/android-luajit-launcher/blob/master/app/build.gradle#L21 for instance). But a new dummy class placed somewhere is needed, as explained before.

Answer 6 · 2021-05-10T21:25:29.000Z

In general I agree with you but in this case is not possible.
...
For features that rely on 3rd party libs this won't work. If the class is generic and relies on google services it will fail to build the flavor where these deps weren't defined.

Yes, of course, it's impossible in one directory. I mean other project structure, for example, main flavor - android/src (leave as is) and fdroid flavor - android/fdroid_src (or even fdroid_stub ?) for fdroid exclusions. Instead of android/app/src/fdroid, android/app/src/gplay, etc.

It is totally possible to have the main sources in current path and just override the class that implements the sync.

OK, you mean override some things with stubs for fdroid flavor. I think it's best way.

If all these methods were implemented on a single class (for example android/src/org/coolreader/sync2/Gdrive.java) ...

Exact path: android/src/org/coolreader/sync2/googledrive/GoogleDriveRemoteAccess.java

In total:

Create fdroid flavor in build.gradle
Change dependencies for fdroid flavor in build.gradle
Create folder android/froid_src (android/src_froid) for fdroid stubs/overrides.
In main sources check BuildConfid and hide Google Drive functionality in interface for fdroid flavor.

Answer 7 · 2021-05-11T02:17:19.000Z

What are the open source libraries for working with Google Drive for Android?

Librera uses some for Google Drive functionality.

Answer 8 · 2021-05-11T02:29:06.000Z

Librera uses some for Google Drive functionality.

They use the same library. Of cource it disabled for fdroid.
https://github.com/foobnix/LibreraReader/blob/master/app/build.gradle#L467

Answer 9 · 2021-05-11T02:40:29.000Z

I can't say much about Librera building process. But F-Droid version has Google Drive sync either.

Answer 10 · 2021-05-17T08:00:13.000Z

If all these methods were implemented on a single class (for example android/src/org/coolreader/sync2/Gdrive.java) it should be possible to leave the class untouched and just add a dummy class android/fdroid/org/coolreader/sync2/Gdrive.java.

No, it's impossible. It produce "duplicate class" error. We must remove this class from main source set and add it into each flavors. https://developer.android.com/studio/build/build-variants#sourceset-build
This is bad, I don't want to split the main version sources into several directories. i.e. for the main version, in any case, we must create some flavor (with some directories for sources) excepting "fdroid" and relocate some of the sources there.

Answer 11 · 2021-05-17T13:07:30.000Z

I would like to solve this

@pazos I thought you'd do it, but I couldn't wait :)
Please test #290, this is what we need?

Answer 12 · 2021-05-17T15:51:41.000Z

No, it's impossible. It produce "duplicate class" error

Oh great. I forgot about that, it seems :(

Please test #290, this is what we need?

Yeah, works great.

I see you added a new Android manifest for the fdroid flavor, which is exactly the same than the main one, except for the <uses-sdk tools:override...>. Maybe we can use the same for both and add a new one on primary with just that part.

Also, another nit is related to donations. They won't be available on the F-Droid market. Would be possible to change about_dialog_donation.xml (used when in-app purchases are not supported) to redirect to a working alternative?. I see current fdroid metadata has https://www.paypal.com/donate/?item_name=Donation+to+Cool+Reader&cmd=_donations&business=coolreader.org%40gmail.com as a donation method.

Answer 13 · 2021-05-17T16:40:16.000Z

I see you added a new Android manifest for the fdroid flavor, which is exactly the same than the main one, except for the <uses-sdk tools:override...>. Maybe we can use the same for both and add a new one on primary with just that part.

By "primary" I mean the main flavor, so I think there should be a minimum difference between the "primary" folder and the "main" folder.

Also, another nit is related to donations. They won't be available on the F-Droid market. Would be possible to change about_dialog_donation.xml (used when in-app purchases are not supported) to redirect to a working alternative?. I see current fdroid metadata has https://www.paypal.com/donate/?item_name=Donation+to+Cool+Reader&cmd=_donations&business=coolreader.org%40gmail.com as a donation method.

I cannot decide this question about donations without @buggins. I need to ask him, of course, it would be ideal for him to make the necessary changes, but, unfortunately, he reduced his activity in the project to a minimum.

Answer 14 · 2021-05-17T17:15:39.000Z

By "primary" I mean the main flavor, so I think there should be a minimum difference between the "primary" folder and the "main" folder.

It is a bit difficult to explain but my suggestion would avoid to have to change stuff twice on both Manifests. Will send a PR to showcase what I'm talking about. Edit: https://github.com/virxkane/coolreader/pull/10

I cannot decide this question about donations without @buggins. I need to ask him, of course, it would be ideal for him to make the necessary changes, but, unfortunately, he reduced his activity in the project to a minimum.

I understand. No problem. That can be changed later if @buggins wish.

The build script of the f-droid repo needs to be updated to fetch 3rd party deps the new way and needs some minor adjustments. I guess we need to wait for buggins too for a proper tagged release.

Answer 15 · 2021-05-17T17:32:55.000Z

About https://github.com/virxkane/coolreader/pull/10:
I understood your idea even before that. This is how I understand: f-droid flavor is an exception, for it I added an exception manifest, primary is the main flavor, I don’t want to add an exception for it, otherwise it will soon turn into a trash heap, we already have android/src, android/app/src/main, android/app/src/primary, android/app/src/fdroid. If we continue to add something to the "primary", it will be easy to get confused where all that is.

Answer 16 · 2021-05-17T17:41:28.000Z

Yup. My point was keeping a single "manifest of truth" in android/app/src/main but I understand your rationale too.

Thanks for the feature, BTW 👍

Answer 17 · 2021-05-18T08:07:45.000Z

@pazos Ok, merged, but you forgot remove the file android/app/src/fdroid/AndroidManifest.xml

Answer 18 · 2021-05-18T20:21:13.000Z

@pazos

Also, another nit is related to donations. They won't be available on the F-Droid market. Would be possible to change about_dialog_donation.xml (used when in-app purchases are not supported) to redirect to a working alternative?. I see current fdroid metadata has https://www.paypal.com/donate/?item_name=Donation+to+Cool+Reader&cmd=_donations&business=coolreader.org%40gmail.com as a donation method.

Fixed in last commits.

Please test again, if there are no other problems I would end on this.

The build script of the f-droid repo needs to be updated to fetch 3rd party deps the new way and needs some minor adjustments. I guess we need to wait for buggins too for a proper tagged release.

This one? https://gitlab.com/fdroid/fdroiddata/-/blob/master/metadata/org.coolreader.yml
Add into prebuild section

cd ../../ && ./thirdparty-deploy.sh && cd android/app/

And somehow specify flavor "f-droid".

Answer 19 · 2021-05-19T23:34:18.000Z

Please test again, if there are no other problems I would end on this.

Sorry for the delay. Everything is fine.

Answer 20 · 2021-08-17T18:41:23.000Z

@pazos Hello. We have new version cr3.2.58 https://github.com/buggins/coolreader/releases/tag/cr3.2.58 with #290.

I would like to solve this

Now it's your turn.
We are looking forward to the new version of CoolReader in the f-droid catalog :)