-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Moves nightly/beta configuration to be a "build type" #1693
Moves nightly/beta configuration to be a "build type" #1693
Conversation
|
||
applicationId "org.mozilla.firefox_beta" | ||
} | ||
firefoxRelease { |
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.
There are flavor specific sources in src
. If we remove those flavors then let's remove the sources too.
app/build.gradle
Outdated
} | ||
greenfieldBeta { | ||
initWith release | ||
applicationIdSuffix ".beta" |
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 we drop the "firefox" variants then let's drop the "greenfield" part here too.
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 100% on board, but be advised that yesterday we talked about keeping the greenfield
moniker around for a bit
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.
Yeah, but this was in conjunction with the firefox
types. If we don't keep them then there's no reason to keep greenfield
. :)
app/build.gradle
Outdated
shrinkResources true | ||
minifyEnabled true | ||
proguardFiles getDefaultProguardFile('proguard-android-optimize.txt'), 'proguard-rules.pro' | ||
matchingFallbacks = ['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.
Interesting that this works. This may need a comment though; otherwise it's confusing: Why does "release" need a fallback on itself?
app/build.gradle
Outdated
applicationIdSuffix ".debug" | ||
manifestPlaceholders.isRaptorEnabled = "true" | ||
} | ||
release { // This isn't deployed to Google Play, but is used as a template for "real" releases like "beta" |
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.
Is the goal to eventually use this for the release build? Having this build type just for the sake of having a template is going to be expensive. Or can the actual "only-release" build be ignored in variantFilter
?
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.
That's a really good point, yeah. I'll represent the template separately, too, so it's not confusing what's reused and what's filtered 👍
af95a31
to
637eb20
Compare
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 a lot for the cleanup. I'm 👍 for these changes. I don't mind changing the routes and letting other teams know. That said, I think we need a new namespace. I left more details below. Tell me what you think!
app/build.gradle
Outdated
} | ||
greenfieldBeta { | ||
initWith release | ||
applicationIdSuffix ".beta" |
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 we decide to change the applicationId
, then we won't be able to ship Fennec Release on the same product as Fennec beta. Per https://docs.google.com/document/d/16lkNScmpnuMCuv38MNGPB2VbMVsa_3B5ryXma6mylSo/edit [section "Fennec Beta (May 1st)], we still don't know what we want to do. I'd recommend to get rid of the suffix for now. @pocmo @colintheshots do you guys see a reason to keep it? I don't know if I'm missing something.
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'll remove it for now. This is editable though (until the beta goes out), so we're flexible for the moment
@@ -57,7 +57,7 @@ def pr_or_push(is_master_push): | |||
build_tasks[assemble_task_id] = BUILDER.craft_assemble_task(variant) | |||
build_tasks[taskcluster.slugId()] = BUILDER.craft_test_task(variant) | |||
|
|||
arch, build_type, _ = _get_architecture_and_build_type_and_product_from_variant(variant) | |||
arch, build_type = _get_architecture_and_build_type_and_product_from_variant(variant) |
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.
Nit: Rename function into _get_architecture_and_build_type_from_variant()
because the product is not returned anymore. Alternative: I'm fine changing the name into something else too.
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 eye, derp
automation/taskcluster/lib/tasks.py
Outdated
artifacts = { | ||
'public/target.{}.apk'.format(arch): { | ||
"type": 'file', | ||
"path": '/opt/fenix/app/build/outputs/apk/' | ||
'{}Greenfield/release/app-{}-greenfield-release-unsigned.apk'.format(arch, arch), | ||
'{}/greenfieldNightly/app-{}-greenfieldNightly-unsigned.apk'.format(arch, arch), |
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.
Nit: I just realized we can reuse https://github.com/mozilla-mobile/fenix/pull/1693/files#diff-59f90d509bc6ae2c9d877250c5ba49c5R458. We can expose a function called _craft_apk_full_path_from_architecture_and_build_type()
and call it here. Can be done in a followup
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.
With the advent of decisionlib
/taskgraph
, I'll leave this for a follow-up
@@ -504,20 +491,7 @@ def _get_architecture_and_build_type_and_product_from_variant(variant): | |||
) | |||
) | |||
|
|||
remaining_variant_data = variant[len(architecture):len(variant) - len(build_type)] |
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.
👍
app/build.gradle
Outdated
manifestPlaceholders.isRaptorEnabled = "true" | ||
} | ||
nightly releaseTemplate | ||
beta releaseTemplate |
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 do we have a different beta buildType? Are we planning to have different store entries like Fennec? Or are we planning to release the release version to beta before release like Focus?
We can push the same binary to beta and then to release, but we'll have to decide how to indicate to users if they're on the beta channel. Google doesn't tell us what track the user is on.
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 question, this was confusing with release
still being in there. Now, we have just nightly
and beta
build types.
How we handle releasing (fennec/focus) is still fluctuating at the moment, so I've got no good answers there :)
124f6c3
to
d2b1b6b
Compare
Bitrise is failing due to variant changes:
If we use |
Looks like there's one merge conflict. |
d2b1b6b
to
0d281fb
Compare
Hey 👋
I was setting up automation work to build a beta, and I noticed that the channels were represented as another axis of "flavors" rather than "build types".
I think that
nightly
andbeta
are better represented asbuild_types
s because:gradle
.Notes:
What do you guys think?
Pull Request checklist