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

:vector-app level variants #6788

Merged
merged 15 commits into from
Sep 16, 2022
Merged

:vector-app level variants #6788

merged 15 commits into from
Sep 16, 2022

Conversation

ouchadam
Copy link
Contributor

@ouchadam ouchadam commented Aug 9, 2022

Type of change

  • Feature
  • Bugfix
  • Technical
  • Other :

Content

  • Lifts the debug, release, gplay and fdroid source-sets to the application module. Allows the majority of the application to be cached between variant builds
  • Enables the gradle build cache by default

Motivation and context

To help improve the compilation time

Screenshots / GIFs

No UI changes

Tests

  • Smoke test with all the different variants/build types

Tested devices

  • Physical
  • Emulator
  • OS version(s): 28

@ouchadam ouchadam added the A-DevX Anything that can improve the DevX label Aug 9, 2022
@ouchadam ouchadam changed the base branch from develop to feature/adm/variants-base August 9, 2022 14:15
@sonarqubecloud
Copy link

sonarqubecloud bot commented Aug 9, 2022

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability C 1 Vulnerability
Security Hotspot E 5 Security Hotspots
Code Smell A 9 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@ouchadam ouchadam changed the base branch from feature/adm/variants-base to feature/adm/strings-module August 11, 2022 14:03
@ouchadam ouchadam changed the base branch from feature/adm/strings-module to feature/adm/variants-base August 11, 2022 14:05
@ouchadam ouchadam force-pushed the feature/adm/app-level-variant-v2 branch from 84e5a38 to 8b5a49c Compare August 11, 2022 14:26
@ouchadam ouchadam changed the base branch from feature/adm/variants-base to feature/adm/strings-module August 11, 2022 14:27
@ouchadam ouchadam force-pushed the feature/adm/strings-module branch 4 times, most recently from b53184b to 22ce829 Compare September 1, 2022 13:58
Base automatically changed from feature/adm/strings-module to develop September 1, 2022 15:11
@ouchadam ouchadam force-pushed the feature/adm/app-level-variant-v2 branch from 8b5a49c to c773eda Compare September 1, 2022 15:26
@ouchadam ouchadam marked this pull request as ready for review September 1, 2022 15:44
Copy link
Member

@bmarty bmarty left a comment

Choose a reason for hiding this comment

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

LGTM, thanks, just one question.

exclude group: 'com.google.android.gms', module: 'play-services-location'
}
fdroidApi(libs.maplibre.pluginAnnotation) {
api(libs.maplibre.pluginAnnotation) {
Copy link
Member

Choose a reason for hiding this comment

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

Will we have a pb here for gplay? group exclusion was not active for this variant. Just to double check.

Copy link
Contributor Author

@ouchadam ouchadam Sep 12, 2022

Choose a reason for hiding this comment

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

it's excluded from the mapLibre dependency and then added back at the application level for the gPlay variant https://github.com/vector-im/element-android/pull/6788/files#diff-6998d5441936ca2baa15113a1f9f709549aeaaf929fedc553b94a67cc9a94833R374

@ouchadam ouchadam force-pushed the feature/adm/app-level-variant-v2 branch from 3800c66 to d1b3104 Compare September 12, 2022 11:17
Copy link
Member

@bmarty bmarty left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the update.

@@ -12,6 +12,7 @@ org.gradle.jvmargs=-Xmx4g -Xms512M -XX:MaxPermSize=2048m -XX:MaxMetaspaceSize=1g
org.gradle.configureondemand=true
org.gradle.parallel=true
org.gradle.vfs.watch=true
org.gradle.caching=true
Copy link
Member

Choose a reason for hiding this comment

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

I have heard that enabling cache can be slower on the CI because it can take more time to retrieve and unzip the cache than download everything again. Do you have any thoughts on 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.

this gradle flag enables the build cache, which relative to the existing gradle cache we're already saving on the CI is only about 300MB~ more

Current CI ~1925 MB https://github.com/vector-im/element-android/runs/8282616819?check_suite_focus=true

With build caching ~2197 MB https://github.com/vector-im/element-android/runs/8309095369?check_suite_focus=true

is about 15 seconds slower~

Copy link
Member

Choose a reason for hiding this comment

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

OK, thanks for checking the duration!


buildFeatures {
viewBinding true
}
Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I had removed this in #7014 because it was not used. There are maybe other changes to revert on my PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll smoke test the different builds 🤞 this was added as it's now used by the debug fragments

Copy link
Contributor Author

Choose a reason for hiding this comment

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

can confirm fdroid, nightly, debug and release builds can log in and send a message

<activity android:name="im.vector.app.features.debug.DebugMenuActivity" />
<activity android:name="im.vector.app.features.debug.leak.DebugMemoryLeaksActivity" />

<activity

Check warning

Code scanning / SonarCloud

Restrict access to exported components with appropriate permissions

<!--SONAR_ISSUE_KEY:AYM9EUYpiMTZgb4KhW9W-->Implement permissions on this exported component. <p>See more on <a href="https://sonarcloud.io/project/issues?id=vector-im_element-android&issues=AYM9EUYpiMTZgb4KhW9W&open=AYM9EUYpiMTZgb4KhW9W&pullRequest=6788">SonarCloud</a></p>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

re-adding comment - this is a debug only manifest ^^^

@ouchadam ouchadam force-pushed the feature/adm/app-level-variant-v2 branch from 8a6ad06 to e9d15b4 Compare September 15, 2022 13:50
@ElementBot
Copy link

ElementBot commented Sep 15, 2022

Warnings
⚠️

vector-app/src/main/AndroidManifest.xml#L8 - The attribute android:allowBackup is deprecated from Android 12 and higher and may be removed in future versions. Consider adding the attribute android:dataExtractionRules specifying an @xml resource which configures cloud backups and device transfers on Android 12 and higher.

⚠️

vector-app/src/main/AndroidManifest.xml#L8 - The attribute android:allowBackup is deprecated from Android 12 and higher and may be removed in future versions. Consider adding the attribute android:dataExtractionRules specifying an @xml resource which configures cloud backups and device transfers on Android 12 and higher.

⚠️

vector/src/main/AndroidManifest.xml#L203 - Consider splitting data tag into multiple tags with individual attributes to avoid confusion

⚠️

vector/src/main/AndroidManifest.xml#L203 - Consider splitting data tag into multiple tags with individual attributes to avoid confusion

⚠️

vector/src/main/AndroidManifest.xml#L206 - Consider splitting data tag into multiple tags with individual attributes to avoid confusion

⚠️

vector/src/main/AndroidManifest.xml#L206 - Consider splitting data tag into multiple tags with individual attributes to avoid confusion

⚠️

vector/src/main/AndroidManifest.xml#L260 - Expecting android:screenOrientation="unspecified" or "fullSensor" for this activity so the user can use the application in any orientation and provide a great experience on Chrome OS devices

⚠️

vector/src/main/AndroidManifest.xml#L260 - Expecting android:screenOrientation="unspecified" or "fullSensor" for this activity so the user can use the application in any orientation and provide a great experience on Chrome OS devices

⚠️

vector/src/main/AndroidManifest.xml#L269 - Attribute supportsPictureInPicture is only used in API level 24 and higher (current min is 21)

⚠️

vector/src/main/AndroidManifest.xml#L269 - Attribute supportsPictureInPicture is only used in API level 24 and higher (current min is 21)

⚠️

vector/src/main/AndroidManifest.xml#L276 - Attribute supportsPictureInPicture is only used in API level 24 and higher (current min is 21)

⚠️

vector/src/main/AndroidManifest.xml#L276 - Attribute supportsPictureInPicture is only used in API level 24 and higher (current min is 21)

⚠️

vector/src/main/AndroidManifest.xml#L282 - Attribute supportsPictureInPicture is only used in API level 24 and higher (current min is 21)

⚠️

vector/src/main/AndroidManifest.xml#L282 - Attribute supportsPictureInPicture is only used in API level 24 and higher (current min is 21)

⚠️

vector/src/main/AndroidManifest.xml#L399 - Exported receiver does not require permission

⚠️

vector/src/main/AndroidManifest.xml#L399 - Exported receiver does not require permission

Generated by 🚫 dangerJS against 60b164a

@sonarqubecloud
Copy link

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability C 1 Vulnerability
Security Hotspot E 3 Security Hotspots
Code Smell A 4 Code Smells

10.9% 10.9% Coverage
0.0% 0.0% Duplication

@ouchadam ouchadam merged commit b05d52b into develop Sep 16, 2022
@ouchadam ouchadam deleted the feature/adm/app-level-variant-v2 branch September 16, 2022 09:36
@ouchadam ouchadam mentioned this pull request Sep 16, 2022
4 tasks
bmarty added a commit that referenced this pull request Sep 19, 2022
We do not use `android-embedded_fcm_distributor` anymore (since #7068).
The code was compiling this `android-embedded_fcm_distributor` has a dependency on `firebase-messaging`.
bmarty added a commit that referenced this pull request Sep 19, 2022
We do not use `android-embedded_fcm_distributor` anymore (since #7068).
The code was compiling because `android-embedded_fcm_distributor` has a dependency on `firebase-messaging`.
bmarty added a commit that referenced this pull request Sep 19, 2022
…regression

Fix regression on our dependency, due to merge of #6788.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-DevX Anything that can improve the DevX
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants