Make Seedvault compile in AOSP properly #98

mikeNG · 2020-07-09T14:50:00Z

Fixes #97

app/build.gradle

chirayudesai · 2020-07-10T22:45:52Z

@stevesoltys do we still want to keep the prebuilt stuff around?

IMO we can drop it in a separate commit, which also ensures that we'll always keep this AOSP build working.

Anyone can still use the prebuilts if they want, we just don't provide an easy way to do it since it's no longer needed.

stevesoltys · 2020-07-10T22:58:31Z

@chirayudesai

I'm away for the weekend so I won't be able to properly review this PR / your suggestion until Sunday, but as long as we still have CI builds working I think we can drop prebuilts without issue.

Though, if there is no additional development / maintenance effort required to keep the prebuilt repo, I don't see the harm in leaving it either. Some people may still find it useful, and they won't have to change their manifests again either.

I can give a more definitive response to that on Sunday, hopefully I'm not blocking anything in the meantime.

grote

I am not an expect on the AOSP build scripts, but the Kotlin changes look good. I suppose you gave the app a quick test to make sure that all views are found and there's no crash being introduced?

stevesoltys

LGTM

stevesoltys · 2020-07-12T23:51:12Z

@chirayudesai

I think that if we can't get CI builds working inside the AOSP source tree (without significant build server resources) it's preferable to continue with prebuilt binaries.

I say this because being able to compile against android.jar is the most lightweight way to validate that (at a minimum) pull requests are able to build in AOSP (as long as all the dependencies are there).

I feel there isn't much maintenance effort required to keep them either. Maybe we can consider removing them if we find that they are a burden to maintain? Let me know if you can think of anything that makes them problematic.

chirayudesai · 2020-07-13T13:38:16Z

Pending test confirmation before merge. Backup / restore / going through settings options should all work.

@stevesoltys Compiling against android.jar won't verify whether it builds in AOSP at all, as the issues here were all due to the build system differences.

I just wanted to make sure everybody used this build in AOSP instead of prebuilts to make sure it got the most testing and stayed updated / working. Prebuilts won't be a burden to maintain I guess, maybe we can add a warning / some info there about this - and mark this as the preferred way?

What I mean is something like: https://www.phoronix.com/scan.php?page=news_item&px=Torvalds-Rust-Kernel-K-Build

grote · 2020-07-13T14:10:51Z

I think we just keep the current CI setup, but I don't think it is related to keeping a prebuilt script around as the CI is doing the gradle build which doesn't say how building in AOSP will work, but at least it can catch other kinds of errors and also run our test cases.

stevesoltys · 2020-07-13T14:34:37Z

@chirayudesai It does validate that the code compiles against AOSP, as long as it's up to date. I'm not referring to actually building in the source tree, which is why I said "as long as the dependencies are there".

Yes, we can change the documentation to prefer this method over prebuilts.

steadfasterX · 2020-07-24T09:51:59Z

I guess that builds just in plain / pure AOSP right?

tried it on LineageOS (16.0) and got

error: packages/apps/Seedvault/Android.bp:48:18: unrecognized property "platform_apis"
error: packages/apps/Seedvault/Android.bp:57:1: unrecognized module type "prebuilt_etc"
error: packages/apps/Seedvault/Android.bp:64:1: unrecognized module type "prebuilt_etc"

mikeNG · 2020-07-25T15:11:09Z

I guess that builds just in plain / pure AOSP right?

tried it on LineageOS (16.0) and got

error: packages/apps/Seedvault/Android.bp:48:18: unrecognized property "platform_apis"
error: packages/apps/Seedvault/Android.bp:57:1: unrecognized module type "prebuilt_etc"
error: packages/apps/Seedvault/Android.bp:64:1: unrecognized module type "prebuilt_etc"

It requires at least Android 10 base to compile properly. Works on AOSP and derivatives like CalyxOS or LineageOS 17.1

relan · 2020-08-05T08:14:03Z

I integrated this change into my custom build of AOSP 10 (not LineageOS derivative). Builds and works for me. Settings, backup, restore---no crashes.

grote · 2020-08-05T12:19:40Z

@relan awesome! Thanks for the feedback!

chirayudesai · 2020-08-10T23:02:28Z

Backup seems to work ok here, but restore from setupwizard via internal storage doesn't.

E AndroidRuntime: kotlin.UninitializedPropertyAccessException: lateinit property backView has not been initialized
E AndroidRuntime:        at com.stevesoltys.seedvault.ui.recoverycode.RecoveryCodeInputFragment.onActivityCreated(RecoveryCodeInputFragment.kt:74)

@relan what provider did you restore from, and through what way did you restore?
Additionally, do you have WITH_DEXPREOPT set for your AOSP build?

chirayudesai

Woops, hit the wrong button there.

Need to go through all variables and double-check that they're all there.

@stevesoltys things like these are exactly why I wanted to get rid of prebuilts and just mandate this for inclusion with AOSP builds. That way it keeps getting tested more. The gradle build would already be used for development, and if somebody wanted they could still use gradle to compile an apk and ship that.

...a/com/stevesoltys/seedvault/ui/recoverycode/RecoveryCodeInputFragment.kt

relan · 2020-08-11T06:25:14Z

what provider did you restore from, and through what way did you restore?

That was internal storage. I launched the RestoreActivity manually (not through the Setup Wizard), it didn't ask for the 12-word password. I reproduced those steps and now I can see it didn't restore app data, only installed APKs. Manual reinstallation (via adb) of APKs does not trigger data restoration despite the Automatic restore option being on.

There are key-related exceptions in the log:

08-11 09:16:57.098 13406 13406 W KeyStore: KeyStore exception
08-11 09:16:57.098 13406 13406 W KeyStore: android.os.ServiceSpecificException:  (code 7)
08-11 09:16:57.098 13406 13406 W KeyStore: 	at android.os.Parcel.createException(Parcel.java:2085)
08-11 09:16:57.098 13406 13406 W KeyStore: 	at android.os.Parcel.readException(Parcel.java:2039)
08-11 09:16:57.098 13406 13406 W KeyStore: 	at android.os.Parcel.readException(Parcel.java:1987)
08-11 09:16:57.098 13406 13406 W KeyStore: 	at android.security.keystore.IKeystoreService$Stub$Proxy.get(IKeystoreService.java:978)
08-11 09:16:57.098 13406 13406 W KeyStore: 	at android.security.KeyStore.get(KeyStore.java:236)
08-11 09:16:57.098 13406 13406 W KeyStore: 	at android.security.KeyStore.get(KeyStore.java:225)
08-11 09:16:57.098 13406 13406 W KeyStore: 	at android.security.keystore.AndroidKeyStoreSpi.engineGetCertificate(AndroidKeyStoreSpi.java:160)
08-11 09:16:57.098 13406 13406 W KeyStore: 	at java.security.KeyStoreSpi.engineEntryInstanceOf(KeyStoreSpi.java:583)
08-11 09:16:57.098 13406 13406 W KeyStore: 	at java.security.KeyStore.entryInstanceOf(KeyStore.java:1631)
08-11 09:16:57.098 13406 13406 W KeyStore: 	at com.stevesoltys.seedvault.crypto.KeyManagerImpl.hasBackupKey(KeyManager.kt:57)
08-11 09:16:57.098 13406 13406 W KeyStore: 	at com.stevesoltys.seedvault.ui.RequireProvisioningViewModel.recoveryCodeIsSet$name(RequireProvisioningViewModel.kt:23)
08-11 09:16:57.098 13406 13406 W KeyStore: 	at com.stevesoltys.seedvault.settings.SettingsActivity.onStart(SettingsActivity.kt:44)
08-11 09:16:57.098 13406 13406 W KeyStore: 	at android.app.Instrumentation.callActivityOnStart(Instrumentation.java:1432)
08-11 09:16:57.098 13406 13406 W KeyStore: 	at android.app.Activity.performStart(Activity.java:7848)
08-11 09:16:57.098 13406 13406 W KeyStore: 	at android.app.ActivityThread.handleStartActivity(ActivityThread.java:3294)
08-11 09:16:57.098 13406 13406 W KeyStore: 	at android.app.servertransaction.TransactionExecutor.performLifecycleSequence(TransactionExecutor.java:221)
08-11 09:16:57.098 13406 13406 W KeyStore: 	at android.app.servertransaction.TransactionExecutor.cycleToPath(TransactionExecutor.java:201)
08-11 09:16:57.098 13406 13406 W KeyStore: 	at android.app.servertransaction.TransactionExecutor.executeLifecycleState(TransactionExecutor.java:173)
08-11 09:16:57.098 13406 13406 W KeyStore: 	at android.app.servertransaction.TransactionExecutor.execute(TransactionExecutor.java:97)
08-11 09:16:57.098 13406 13406 W KeyStore: 	at android.app.ActivityThread$H.handleMessage(ActivityThread.java:2016)
08-11 09:16:57.098 13406 13406 W KeyStore: 	at android.os.Handler.dispatchMessage(Handler.java:107)
08-11 09:16:57.098 13406 13406 W KeyStore: 	at android.os.Looper.loop(Looper.java:214)
08-11 09:16:57.098 13406 13406 W KeyStore: 	at android.app.ActivityThread.main(ActivityThread.java:7356)
08-11 09:16:57.098 13406 13406 W KeyStore: 	at java.lang.reflect.Method.invoke(Native Method)
08-11 09:16:57.098 13406 13406 W KeyStore: 	at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:492)
08-11 09:16:57.098 13406 13406 W KeyStore: 	at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:930)

Additionally, do you have WITH_DEXPREOPT set for your AOSP build?

I kept the default, so it should be on.

chirayudesai · 2020-08-11T11:46:04Z

what provider did you restore from, and through what way did you restore?

That was internal storage. I launched the RestoreActivity manually (not through the Setup Wizard), it didn't ask for the 12-word password. I reproduced those steps and now I can see it didn't restore app data, only installed APKs. Manual reinstallation (via adb) of APKs does not trigger data restoration despite the Automatic restore option being on.

Ok, not asking for the password sounds like you didn't wipe, that's expected if so.
Not restoring app data is strange, here that works fine (I specifically changed some stuff like theme/wallpaper/to make it easier to verify.

We're on #seedvault on freenode if you want to discuss anything real-time btw.

stevesoltys · 2020-08-12T19:52:17Z

@stevesoltys things like these are exactly why I wanted to get rid of prebuilts and just mandate this for inclusion with AOSP builds. That way it keeps getting tested more. The gradle build would already be used for development, and if somebody wanted they could still use gradle to compile an apk and ship that.

@chirayudesai I'm not understanding what you ran into / how prebuilts caused this. Could you explain please?

chirayudesai · 2020-08-12T20:29:32Z

My initial test with this had issues, but I retried afterwards and it seemed fine.

Scenarios to test:

Upgrade device to Android.bp, trying to make a new backup
New install, try to restore an old gradle backup.
New install, try to backup.

@stevesoltys things like these are exactly why I wanted to get rid of prebuilts and just mandate this for inclusion with AOSP builds. That way it keeps getting tested more. The gradle build would already be used for development, and if somebody wanted they could still use gradle to compile an apk and ship that.

@chirayudesai I'm not understanding what you ran into / how prebuilts caused this. Could you explain please?

Prebuilts didn't cause any issues, but Android.bp may have.

My point was we should standardize on Android.bp to make sure it gets tested well all the time.

stevesoltys · 2020-08-15T06:27:57Z

@chirayudesai Ah, okay. We can prebuilts if you think that's preferable to ensure this new build method is well tested. I think I misunderstood before.

mikeNG · 2020-08-24T05:59:06Z

@chirayudesai I took a look and I don't see any other missing variable


        Drop dependency on kotlin-android-extensions

* Not available on AOSP.


        Upgrade androidx-lifecycle-livedata to 2.3.0-alpha05

* All previous aar versions have their modified date set to 0 which trigger the following openjdk bug: https://bugs.openjdk.java.net/browse/JDK-8184940 This fixes the following compilation error while building in AOSP environment: java.time.DateTimeException: Invalid value for MonthOfYear (valid values 1 - 12): 0


        Make seedvault compile in AOSP properly

* Add Android.bp to compile using AOSP build system instead of gradle * Add prebuilt external libs that are not available on AOSP Fixes #97 Co-authored-by: Chirayu Desai <chirayudesai1@gmail.com>

mikeNG · 2020-09-07T17:33:41Z

I have tested the following a few times and I couldn't find any bugs caused by the Android.bp conversion:

make backup with unmodified build - backup-step-1
upgrade (no wipe) to Android.bp build
make another backup, backup-step-3
wipe
try to restore backup-step-1
wipe
try to restore backup-step-3
wipe
create a new backup, fresh device - backup-step-9


        Merge branch 'develop' into develop

chirayudesai · 2020-09-08T13:43:18Z

Can merge this after a quick smoke test by @mikeNG

It should compile now, I had github auto update - it merged, but that's fine since we're rebasing pull requests so we won't have double merge commits.

mikeNG · 2020-09-08T19:25:49Z

I have tested backup/wipe/restore and it still works as before. This can be merged.

chirayudesai marked this pull request as draft Jul 9, 2020

mikeNG force-pushed the mikeNG:develop branch 3 times, most recently from d93b4ef to 5848659 Jul 10, 2020

chirayudesai changed the title ~~Non-working Android.bp~~ Jul 10, 2020

chirayudesai reviewed Jul 10, 2020

View changes

app/build.gradle Show resolved Hide resolved

chirayudesai marked this pull request as ready for review Jul 10, 2020

mikeNG force-pushed the mikeNG:develop branch from 5848659 to be6b733 Jul 10, 2020

grote reviewed Jul 11, 2020

View changes

mikeNG dismissed grote’s stale review via 40ae860 Jul 12, 2020

mikeNG force-pushed the mikeNG:develop branch from be6b733 to 40ae860 Jul 12, 2020

stevesoltys reviewed Jul 12, 2020

View changes

chirayudesai mentioned this pull request Aug 10, 2020

Add instrumentation tests for storage plugin (SAF) #107

Merged

chirayudesai marked this pull request as draft Aug 10, 2020

chirayudesai closed this Aug 10, 2020

chirayudesai reopened this Aug 10, 2020

chirayudesai requested changes Aug 10, 2020

View changes

...a/com/stevesoltys/seedvault/ui/recoverycode/RecoveryCodeInputFragment.kt Show resolved Hide resolved

chirayudesai marked this pull request as ready for review Aug 11, 2020

chirayudesai dismissed stevesoltys’s stale review via d027aa8 Aug 11, 2020

chirayudesai changed the title ~~Make Seedvault compile in AOSP properly~~ Aug 11, 2020

chirayudesai mentioned this pull request Aug 26, 2020

Merge develop into master #110

Closed

grote mentioned this pull request Sep 2, 2020

Adapt to Android 11 AOSP changes #94

Open

mikeNG and others added 3 commits Jul 9, 2020

Drop dependency on kotlin-android-extensions

2460cbf

* Not available on AOSP.

Make seedvault compile in AOSP properly

Loading status checks…

7f3ee00

* Add Android.bp to compile using AOSP build system instead of gradle * Add prebuilt external libs that are not available on AOSP Fixes #97 Co-authored-by: Chirayu Desai <chirayudesai1@gmail.com>

mikeNG force-pushed the mikeNG:develop branch from d027aa8 to 7f3ee00 Sep 7, 2020

chirayudesai approved these changes Sep 7, 2020

View changes

grote changed the title ~~[DO NOT MERGE] Make Seedvault compile in AOSP properly~~ Sep 7, 2020

grote self-requested a review Sep 7, 2020

grote approved these changes Sep 7, 2020

View changes

grote changed the title ~~Make Seedvault compile in AOSP properly~~ Sep 7, 2020

Merge branch 'develop' into develop

Verified

This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.

GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits

Loading status checks…

73b23d7

chirayudesai changed the title ~~DO NOT MERGE: Make Seedvault compile in AOSP properly~~ Sep 8, 2020

chirayudesai merged commit c40c694 into stevesoltys:develop Sep 8, 2020
1 check passed

1 check passed

Travis CI - Pull Request Build Passed
Details

Aug	SEP	Oct
	20
2019	2020	2021

stevesoltys / seedvault

Make Seedvault compile in AOSP properly #98

Make Seedvault compile in AOSP properly #98

mikeNG commented Jul 9, 2020 •

edited

chirayudesai commented Jul 10, 2020

stevesoltys commented Jul 10, 2020

grote left a comment

stevesoltys left a comment

stevesoltys commented Jul 12, 2020

chirayudesai commented Jul 13, 2020

grote commented Jul 13, 2020

stevesoltys commented Jul 13, 2020

steadfasterX commented Jul 24, 2020 •

edited

mikeNG commented Jul 25, 2020

relan commented Aug 5, 2020

grote commented Aug 5, 2020

chirayudesai commented Aug 10, 2020

chirayudesai left a comment •

edited

relan commented Aug 11, 2020

chirayudesai commented Aug 11, 2020

stevesoltys commented Aug 12, 2020

chirayudesai commented Aug 12, 2020

stevesoltys commented Aug 15, 2020

mikeNG commented Aug 24, 2020

mikeNG commented Sep 7, 2020

chirayudesai commented Sep 8, 2020

mikeNG commented Sep 8, 2020

stevesoltys / seedvault

Join GitHub today

Make Seedvault compile in AOSP properly #98

Make Seedvault compile in AOSP properly #98

Conversation

mikeNG commented Jul 9, 2020 • edited

chirayudesai commented Jul 10, 2020

stevesoltys commented Jul 10, 2020

grote left a comment

stevesoltys left a comment

stevesoltys commented Jul 12, 2020

chirayudesai commented Jul 13, 2020

grote commented Jul 13, 2020

stevesoltys commented Jul 13, 2020

steadfasterX commented Jul 24, 2020 • edited

mikeNG commented Jul 25, 2020

relan commented Aug 5, 2020

grote commented Aug 5, 2020

chirayudesai commented Aug 10, 2020

chirayudesai left a comment • edited

relan commented Aug 11, 2020

chirayudesai commented Aug 11, 2020

stevesoltys commented Aug 12, 2020

chirayudesai commented Aug 12, 2020

stevesoltys commented Aug 15, 2020

mikeNG commented Aug 24, 2020

mikeNG commented Sep 7, 2020

chirayudesai commented Sep 8, 2020

mikeNG commented Sep 8, 2020

mikeNG commented Jul 9, 2020 •

edited

steadfasterX commented Jul 24, 2020 •

edited

chirayudesai left a comment •

edited