Skip to content
This repository has been archived by the owner on Nov 1, 2022. It is now read-only.

Migrate the Glean SDK to the Rust implementation #4620

Merged
merged 1 commit into from
Oct 24, 2019

Conversation

Dexterp37
Copy link
Contributor

@Dexterp37 Dexterp37 commented Oct 3, 2019

This completely removes the Kotlin implementation of the Glean SDK from this repository. It introduces aliases that are used instead.

Please note that integration tests keep living in this repository as part of the Glean sample app.

Note: this is a massive PR with deletion, mostly. I'd recommend reviewing it commit-by-commit

TODO

  • remove the glean binary;
  • depend on the maven version of the Glean SDK.

Pull Request checklist

  • Quality: This PR builds and passes detekt/ktlint checks (A pre-push hook is recommended)
  • Tests: This PR includes thorough tests or an explanation of why it does not
  • Changelog: This PR includes a changelog entry or does not need one
  • Accessibility: The code in this PR follows accessibility best practices or does not include any user facing features

After merge

  • Milestone: Make sure issues closed by this pull request are added to the milestone of the version currently in development.
  • Breaking Changes: If this is a breaking change, please push a draft PR on Reference Browser to address the breaking issues.

@Dexterp37 Dexterp37 self-assigned this Oct 3, 2019
Copy link
Contributor

@mdboom mdboom left a comment

Choose a reason for hiding this comment

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

+r, modulo working through the ktlint/detekt errors.

Copy link
Member

@travis79 travis79 left a comment

Choose a reason for hiding this comment

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

Is adding the binary glean-0.0.1-TESTING1.aar to source control intentional?

@Dexterp37
Copy link
Contributor Author

Is adding the binary glean-0.0.1-TESTING1.aar to source control intentional?

Yes, it's meant to be removed after the library is available on Maven (see the TODO in the PR description)

@travis79
Copy link
Member

travis79 commented Oct 3, 2019

Is adding the binary glean-0.0.1-TESTING1.aar to source control intentional?

Yes, it's meant to be removed after the library is available on Maven (see the TODO in the PR description)

Sorry, I totally overlooked the TODO. This makes sense, I just thought it was weird to include a binary and wanted to make sure.

@Dexterp37 Dexterp37 force-pushed the gleancore_migration branch 2 times, most recently from 5409fda to 54a6643 Compare October 7, 2019 16:00
@codecov
Copy link

codecov bot commented Oct 7, 2019

Codecov Report

Merging #4620 into master will decrease coverage by 0.88%.
The diff coverage is 0%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #4620      +/-   ##
============================================
- Coverage     81.35%   80.47%   -0.89%     
+ Complexity     4377     3907     -470     
============================================
  Files           561      511      -50     
  Lines         19087    17190    -1897     
  Branches       2782     2514     -268     
============================================
- Hits          15529    13833    -1696     
+ Misses         2439     2339     -100     
+ Partials       1119     1018     -101
Impacted Files Coverage Δ Complexity Δ
...ents/service/glean/net/ConceptFetchHttpUploader.kt 100% <ø> (ø) 7 <0> (ø) ⬇️
...a/components/service/glean/config/Configuration.kt 0% <0%> (-100%) 0 <0> (-10)
...ain/java/mozilla/components/service/glean/Glean.kt 0% <0%> (-84.44%) 0 <0> (-1)
...s/lib/push/firebase/AbstractFirebasePushService.kt 70.37% <0%> (-3.71%) 8% <0%> (ø)
...ponents/service/location/MozillaLocationService.kt
...ocation/search/RegionSearchLocalizationProvider.kt

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 321d499...12163f1. Read the comment docs.

@Dexterp37 Dexterp37 force-pushed the gleancore_migration branch from 5aacd7a to 360a47c Compare October 8, 2019 10:11
@Dexterp37 Dexterp37 force-pushed the gleancore_migration branch 2 times, most recently from 1f4cab0 to ae0a1f3 Compare October 9, 2019 09:12
@Dexterp37 Dexterp37 requested a review from badboy October 9, 2019 09:18
@Dexterp37 Dexterp37 force-pushed the gleancore_migration branch 4 times, most recently from 7aaed8a to 766a6b9 Compare October 16, 2019 11:48
@Dexterp37 Dexterp37 force-pushed the gleancore_migration branch 3 times, most recently from 1e06c40 to 82620f9 Compare October 22, 2019 09:28
Copy link
Member

@badboy badboy left a comment

Choose a reason for hiding this comment

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

I built locally against this PR and even generated the Fenix APKs we used for testing and QA.
This seems to be all working.

The remaining steps:

  • Remove the mavenLocal() line
  • Wait for our Maven release
  • Re-base and push this PR again to trigger CI
  • Squash the commits into something reasonable

This removes the Kotlin implementation from this repository
and introduces type aliases that point to the Rust implementation,
which is now a new dependency.
@Dexterp37 Dexterp37 force-pushed the gleancore_migration branch from 661a33e to 12163f1 Compare October 24, 2019 08:07
@Dexterp37
Copy link
Contributor Author

bors=mdboom,travis79,badboy
bors r+

bors bot pushed a commit that referenced this pull request Oct 24, 2019
4620: Migrate the Glean SDK to the Rust implementation r=Dexterp37 a=Dexterp37

This completely removes the Kotlin implementation of the Glean SDK from this repository. It introduces aliases that are used instead.

Please note that integration tests keep living in this repository as part of the [Glean sample app](https://github.com/mozilla-mobile/android-components/tree/master/samples/glean/src/androidTest/java/org/mozilla/samples/glean).

**Note: this is a massive PR with deletion, mostly. I'd recommend reviewing it commit-by-commit**

**TODO**

- [x] remove the glean binary;
- [x] depend on the maven version of the Glean SDK.



Co-authored-by: Alessio Placitelli <[email protected]>
@bors
Copy link

bors bot commented Oct 24, 2019

Build succeeded

  • complete-push

@bors bors bot merged commit 12163f1 into mozilla-mobile:master Oct 24, 2019
bors bot pushed a commit that referenced this pull request Oct 30, 2019
4892: Add back the @jvmoverloads to the Glean SDK public API r=Dexterp37 a=Dexterp37

This additionally adds unit testing in Java to make sure we're not regressing this again.

Note: this restores the annotations that were removed by #4620



Co-authored-by: Alessio Placitelli <[email protected]>
bors bot pushed a commit that referenced this pull request Oct 30, 2019
4892: Add back the @jvmoverloads to the Glean SDK public API r=Dexterp37 a=Dexterp37

This additionally adds unit testing in Java to make sure we're not regressing this again.

Note: this restores the annotations that were removed by #4620



Co-authored-by: Alessio Placitelli <[email protected]>
bors bot pushed a commit that referenced this pull request Oct 30, 2019
4892: Add back the @jvmoverloads to the Glean SDK public API r=Dexterp37 a=Dexterp37

This additionally adds unit testing in Java to make sure we're not regressing this again.

Note: this restores the annotations that were removed by #4620



Co-authored-by: Alessio Placitelli <[email protected]>
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.

4 participants