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

Upgrade AGP, Gradle, language dependencies #75

Merged
merged 7 commits into from
Nov 16, 2022

Conversation

prudrabhat
Copy link
Contributor

@prudrabhat prudrabhat commented Nov 8, 2022

Description

  • Upgraded tools & dependancies
    • Gradle distribution 7.5
    • Gradle 7.3.0
    • Java 1.8
    • Kotlin 1.6.21
    • jacoco 0.8.8
  • Expanded .gitignore to include more files.
  • Restructure plugin declarations

Related Issue

MOB-17088

Motivation and Context

Upgrade tools and dependancies in preparation for Android Core 2.0 adoption.

How Has This Been Tested?

  • Verified that ci-build , ci-javadoc ci-unit-test ci-funtional-testpass locally.

Screenshots (if appropriate):

N/A

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • I have signed the Adobe Open Source CLA.
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

- Upgrades
   - Gradle distribution 7.5
   - Gradle 7.3.1
   - Java 1.8
   - Kotlin 1.6.2
   - jacoco 0.8.8
- Expand .gitignore
- Restructure plugin declarations
- Fix functional test configuration (dex, androidx.test:core))
@prudrabhat prudrabhat force-pushed the dev-v2.0.0_setup branch 2 times, most recently from f4553b5 to d4344a8 Compare November 8, 2022 19:57
@prudrabhat prudrabhat marked this pull request as ready for review November 8, 2022 20:48
code/app/build.gradle Outdated Show resolved Hide resolved
code/app/build.gradle Outdated Show resolved Hide resolved
code/app/build.gradle Outdated Show resolved Hide resolved
code/build.gradle Show resolved Hide resolved
code/build.gradle Outdated Show resolved Hide resolved
code/edgeidentity/build.gradle Outdated Show resolved Hide resolved
code/edgeidentity/build.gradle Show resolved Hide resolved
code/edgeidentity/build.gradle Show resolved Hide resolved
code/edgeidentity/build.gradle Show resolved Hide resolved
code/edgeidentity/build.gradle Outdated Show resolved Hide resolved
code/edgeidentity/build.gradle Outdated Show resolved Hide resolved
- remove *Version suffix for sdk options in gradle
- switch to gradle 7.3.0
- Use `required` instead of `enabled` for xml, csv, html reports
- Remove aar maven upload script
@cacheung
Copy link
Contributor

cacheung commented Nov 9, 2022

We have another AndroidManifest.xml at
https://github.com/prudrabhat/aepsdk-edgeidentity-android/blob/dev-v2.0.0_setup/code/edgeidentity/src/main/AndroidManifest.xml

@codecov
Copy link

codecov bot commented Nov 9, 2022

Codecov Report

Merging #75 (4973860) into dev-v2.0.0 (7338285) will increase coverage by 0.40%.
The diff coverage is n/a.

@@              Coverage Diff               @@
##           dev-v2.0.0      #75      +/-   ##
==============================================
+ Coverage       81.30%   81.69%   +0.40%     
==============================================
  Files              20       20              
  Lines            1032     1027       -5     
  Branches          151      151              
==============================================
  Hits              839      839              
+ Misses            137      132       -5     
  Partials           56       56              
Flag Coverage Δ
functional-tests 60.95% <ø> (+0.30%) ⬆️
unit-tests 79.16% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...keting/mobile/edge/identity/IdentityConstants.java 0.00% <0.00%> (ø)
...keting/mobile/edge/identity/IdentityExtension.java 77.84% <0.00%> (+0.44%) ⬆️
...adobe/marketing/mobile/edge/identity/Identity.java 72.87% <0.00%> (+0.56%) ⬆️
...om/adobe/marketing/mobile/edge/identity/Utils.java 77.42% <0.00%> (+1.23%) ⬆️
...g/mobile/edge/identity/IdentityStorageService.java 93.88% <0.00%> (+1.88%) ⬆️

Copy link
Contributor

@cacheung cacheung left a comment

Choose a reason for hiding this comment

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

Looks good. Have a question about multiDexEnabled.

Functional tests will fail without this flag as this module exceeds the DEX limit.
It is sufficient to enable this for debug as it is the type used for functional tests.
Consuming apps are responsible for making changes to their gradle to overcome
the DEX limit as necessary.
Copy link
Collaborator

@kevinlind kevinlind left a comment

Choose a reason for hiding this comment

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

In code/edgeidentity/build.gradle please update publishing/publications/release with the following:

pom {
    url =  'https://developer.adobe.com/client-sdks'
}
   scm {
        connection = 'scm:git:github.com//adobe/aepsdk-edgeidentity-android.git'
        developerConnection = 'scm:git:ssh://github.com//adobe/aepsdk-edgeidentity-android.git'
        url = 'https://github.com/adobe/aepsdk-edgeidentity-android'
    }

Comment on lines 30 to 31
// build tools
buildToolsVersion = "29.0.3"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Setting the build tools version shouldn't be required anymore, can you remove this?

// dependencies
junitVersion = "1.1.2"
// kotlin config
kotlinVersion = "1.6.21"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't see this kotlinVersion being used, can you remove it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is being used in code/app/build.gradle Line#48.

Comment on lines +34 to +35
prettierVersion = "2.7.1"
prettierPluginJavaVersion = "1.6.2"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you use these variables in the Spotless configuration in code/edgeidentity/build.gradle line 20?

// It is sufficient to enable this for debug as it is the type used for functional tests.
// Consuming apps are responsible for making changes to their gradle to overcome
// the DEX limit as necessary.
multiDexEnabled true
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you can remove this if you remove the dependencies implementation 'androidx.appcompat:appcompat:1.2.0' and androidTestImplementation "androidx.test:core:1.4.0". In my testing neither dependency was needed.

However, when I also updated com.fasterxml.jackson.core:jackson-databind to 2.12.7 and androidx.test.espresso:espresso-core to 3.5.0, then I went over the dex limit again by about 20 methods. At this point I'd suggest removing multidex and the to unneeded dependencies.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are correct! Both of these dependencies are not needed for this project as well and removing it reduced the method count below dex limit.

@kevinlind kevinlind merged commit 10c43ea into adobe:dev-v2.0.0 Nov 16, 2022
@prudrabhat prudrabhat deleted the dev-v2.0.0_setup branch November 17, 2022 01:43
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.

3 participants