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
Wear configuration demo #24
Conversation
} | ||
|
||
qaRelease.initWith(buildTypes.release) | ||
qaRelease { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe just qa
? I wonder if this will break the smart task abbreviation because of the clashing with release
😕
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea for simplify to just qa
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -3,7 +3,7 @@ buildscript { | |||
jcenter() | |||
} | |||
dependencies { | |||
classpath 'com.android.tools.build:gradle:1.0.0' | |||
classpath 'com.android.tools.build:gradle:2.2.1' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why these changes to other modules? I see we have a root multi-module project, but what's the value of it? ideally all these demos should be separated project, right?
If anything we should consider to extract this common dependency (or at least the version) in a single place where it will be easier to maintain
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regarding the changes, these were done automatically by the IDE, i just thought there was no harm in updating in other modules too.
Regarding the multi-module, I can see it useful to reduce duplication of the gradle wrapper for example, and also the fact that it's easier to load it immediately from the root, having access to all the projects.
This is just my point of view, I don't remember the historical reasons behind this repo structure honestly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My considerations comes from the realisation every module is an app on its own. I see the plus of the simpler import, but that means also longer evaluation times (the whole root project is evaluated in the process 😅 ). I'm not saying you should change the rest of the projects, but I would seriously consider not to follow the (broken IMHO) pattern and make this a separate project on its own (all you need is avoid to list your module in the root settings.gradle
, right?).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I see your point.
Honestly I don't mind having this one as an independent project, but that would require a duplication of the gradle wrapper or can we avoid that somehow?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nah, duplication of the wrapper can't be avoided, but I really don't feel is like a big deal 😸 Actually this is a good opportunity to use Gradle 3.x. in your own isolated and cohesive project. Mmm 🤔 sounds even better to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, followed your suggestion @mr-archano and moved this demo into a independent project: dec78a2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not updating to 2.2.3 while we're on it? 2.2.1 wasn't the latest even 15 days ago :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
¯_(ツ)_/¯ it was not the main point of this PR
@@ -0,0 +1,71 @@ | |||
apply plugin: 'com.android.application' | |||
apply from: "$rootProject.projectDir/WearBuildConfig/shared-config/android-signing.gradle" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this could be a more idiomatic rootProject.file('WearBuildConfig/shared-config/android-signing.gradle')
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 3f4cf8d
versionCode 1 | ||
versionName "1.0" | ||
|
||
buildConfigField 'boolean', 'PLAY_SERVICES', "true" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you're seeking a good example of a build.gradle
you could consider the use of the gradle-build-properties-plugin
and its typed decorators for buildConfigField
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the hint, but this field is just an example of a value set differently in the two variations of the flavour dimension.
I'd prefer to keep it as simple as possible, without adding external plugins in this demo
//uses default PLAY_SERVICES value | ||
} | ||
|
||
preProduction { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is preProduction
? maybe development
as environment would be easier to grasp
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this project is to show an example of second flavour dimension: the environment.
Agree, development
could be easier to understand
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 042ae78
} | ||
|
||
dependencies { | ||
productionPlayServicesReleaseWearApp project(path: ':WearBuildConfigWear', configuration: 'productionRelease') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm lost. Above you defined 4 additional configurations, but they've not be used anywhere else, right? Maybe this is material for the (missing) README
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These configurations are used under the hood by gradle to copy the Wear APK into the mobile APK's res/raw folder.
This is done ONLY for the configurations here defined, so for example it doesn't happen for noPlayServices
builds.
Good point on the readme, I'll write one when I'm back
@@ -26,3 +26,7 @@ include 'TabHostExample' | |||
include 'TabHostSelfContainedTabBrowsing' | |||
include 'wizard' | |||
include 'EditTextChips' | |||
include 'WearBuildConfigMobile' | |||
project(':WearBuildConfigMobile').projectDir = new File('WearBuildConfig/mobile') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, I think I see what you're trying to do, but I'm still not sure why you need to create this alias (and the one below). If you're trying to workaround the multi module setup in the whole android-demos
maybe this is not the best solution. clarifying your intent here would help a lot :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I'm trying to do is to create two modules that share the same parent in the main project root:
- WearBuildConfig/mobile
- WearBuildConfig/wear
Can you specify what's wrong with this way of doing it?
This is the same strategy it was used for other modules here and I'll be curious to hear about alternatives (even if we might make this project independent)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I wasn't clear enough: read my comment above as something in line with the suggestion of make this module a self-contained project. It's not wrong, it follows what's already in place, but I personally think it's a flawed approach that we should address soon.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💯 nice one @danybony !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just wondering why we've upgraded to old tools versions instead of the latest, but can ship as-is
@@ -3,7 +3,7 @@ buildscript { | |||
jcenter() | |||
} | |||
dependencies { | |||
classpath 'com.android.tools.build:gradle:1.0.0' | |||
classpath 'com.android.tools.build:gradle:2.2.1' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not updating to 2.2.3 while we're on it? 2.2.1 wasn't the latest even 15 days ago :D
distributionPath=wrapper/dists | ||
zipStoreBase=GRADLE_USER_HOME | ||
zipStorePath=wrapper/dists | ||
distributionUrl=https\://services.gradle.org/distributions/gradle-3.0-all.zip |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Y U NO 3.2.1?
|
||
android { | ||
compileSdkVersion 25 | ||
buildToolsVersion "25.0.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not 25.0.1 while we're at it?
This PR adds a demo build config needed to add a Wear module to an existing project.
This demo shows how to integrate Wear with some real-world scenario like