Skip to content
This repository has been archived by the owner on Dec 14, 2021. It is now read-only.

Use 'org.mozilla.appservices' Gradle plugin to consume single megazor… #353

Merged
merged 2 commits into from
Jan 9, 2019

Conversation

ncalexan
Copy link
Contributor

@ncalexan ncalexan commented Jan 7, 2019

…d lib. (#352)

Fixes #352
Fixes #242

Related to #242, #337

I notice that Lockbox is currently configured for x86_64, but I don't think that a-s is producing any x86_64 binaries: mozilla/application-services#426. If we're confident that x86_64 support is needed, this is blocked by that ticket. If not, I think we can land this without x86_64 support, since it's the status quo.

Testing and Review Notes

This follows the Reference Browser model and the documentation.

This needs to be tested (manually, or on emulators that actually open SQLite DBs) on devices of different architectures:

  • armeabi-v7a
  • arm64-v8a
  • x86
  • maybe x86_64 (see above)

It works fine on my armeabi-v7a device (an old Galaxy S6, I think).

Sorry, something went wrong.

@ncalexan ncalexan requested a review from a team as a code owner January 7, 2019 20:06
@ghost ghost added the in progress label Jan 7, 2019
@linuxwolf
Copy link
Contributor

@ncalexan thanks!

I notice that Lockbox is currently configured for x86_64, but I don't think that a-s is producing any x86_64 binaries

I don't think we have any important reason for requiring x86_64. I think we could switch to x86 without penalty.

Thoughts @sashei @jhugman ?

@sashei
Copy link
Contributor

sashei commented Jan 8, 2019

I'm curious whether this will impact anyone's emulator / on-computer usage? But barring that I see no reason to keep x86_64

@ncalexan
Copy link
Contributor Author

ncalexan commented Jan 8, 2019

I'm curious whether this will impact anyone's emulator / on-computer usage? But barring that I see no reason to keep x86_64

If anybody is running an x86_64 emulator, it might -- but AFAICT such a user would already hard-crash due to missing native libraries.

So all told I think that means this can Just Land (after manual testing, please!) without regressing your architecture story, and we can pursue x86_64 more thoroughly in parallel.

Copy link
Contributor

@sashei sashei left a comment

Choose a reason for hiding this comment

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

🤖 🎉

@ghost ghost assigned sashei Jan 8, 2019
@sashei sashei removed their assignment Jan 8, 2019
@devinreams
Copy link
Contributor

So all told I think that means this can Just Land (after manual testing, please!) without regressing your architecture story, and we can pursue x86_64 more thoroughly in parallel.

Thanks @ncalexan! Unless anyone objects, I will do some manual/device testing tomorrow-ish on this branch and merge assuming everything looks OK.

@devinreams devinreams self-requested a review January 8, 2019 23:52
Copy link
Contributor

@devinreams devinreams left a comment

Choose a reason for hiding this comment

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

Tested Build 2054 both on Pixel 2 and this big Samsung Galaxy Note 5 and successfully: signed in, synced real data, re-sorted list, loaded FxA avatar and display name, opened webviews, locked and unlocked. ✅

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants