Closed Bug 1144463 Opened 5 years ago Closed 5 years ago

Add dolphin-512 config

Categories

(Taskcluster :: General, defect)

x86
macOS
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: jlal, Assigned: seinlin)

Details

Attachments

(2 files, 5 obsolete files)

No description provided.
/r/5577 - Bug 1144463 - Testing adding dolphin r=kli

Pull down this commit:

hg pull review -r 8a5beabcf69ff4fae6e8605f34e7b7e95f19eaeb
Attachment #8579044 - Flags: review?(kli)
James, I can successfully build on my local machine. Could you review this patch? 

I also push to try [1], but it is pending for a long time and the build is not started. Should I push 'quay.io/mozilla/builder:0.5.3' before I test on try?

[1] https://treeherder.allizom.org/#/jobs?repo=try&revision=68f3b8bf5710
Attachment #8579044 - Attachment is obsolete: true
Attachment #8579044 - Flags: review?(kli)
Attachment #8581207 - Flags: review?(jlal)
The problem here is simple at this point we need to setup a worker type on http://aws-provisioner.taskcluster.net/ called "dolphin-512" those types are not automatically created (you can copy the setup for emulator-kk for example since we don't need anything special for dolphin builds like we do for the flames). I will take a look my Monday if you still need help.
Assignee: jlal → kli
I created a work type called dolphin-512 and re-trigger the task, but it is still pending.
http://docs.taskcluster.net/tools/task-inspector/#9BRf4tkAQ8m1-4s4LrZq9w/0

In task definition, builder 0.5.3 is required. I build it on my local machine, but I didn't push to quay.io. Do I need to push it? Or Will it be built from the patch? All changes of builder is included in patch which is pushed to try.
Flags: needinfo?(jlal)
Now the task is failed, I can see quay.io/mozilla/builder:0.5.3 is downloaded. A new problem is encountered.
You will indeed need to push it to quay.io
Flags: needinfo?(jlal)
Comment on attachment 8581207 [details] [diff] [review]
Add dolphin-512 and update docker

r- only because I think ./build-dolphin.sh is missing (that is why your task failed) ?
Attachment #8581207 - Flags: review?(jlal) → review-
James, Now the builds are green [1]. Could you review the patch?
In new patch,  instead of checkout l10n repos, devices' language.json is passed to b2g_build.py as parameter.

https://treeherder.allizom.org/#/jobs?repo=try&revision=6cdf462c8c2f
Attachment #8585124 - Flags: review?(jlal)
Comment on attachment 8585124 [details] [diff] [review]
Add dolphin-512 and update docker

Review of attachment 8585124 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/docker/builder/VERSION
@@ -1,1 @@
> -0.5.2

(reminder) prior to landing can you please verify that we are not clobbering a version that already exists?

::: testing/taskcluster/scripts/phone-builder/build-dolphin.sh
@@ +33,5 @@
> +
> +# L10N_LIST=`grep ':' $WORKSPACE/B2G/device/sprd/scx15/languages.json | cut -d '"' -f 2`
> +# for l10n in $L10N_LIST; do
> +# # Checkout l10n repos
> +# tc-vcs checkout $WORKSPACE/B2G/gaia-l10n/$l10n https://hg.mozilla.org/releases/gaia-l10n/v2_1/$l10n

looks like we can remove this right ?

::: testing/taskcluster/tasks/builds/b2g_dolphin_512_eng.yml
@@ +3,5 @@
> +task:
> +  scopes:
> +    - 'docker-worker:cache:build-dolphin-512-eng'
> +  metadata:
> +    name: B2G Dolphin 512 Eng

Can you prefix name with [TC] ( makes it clear to sheriffs / etc... what built this thing )

::: testing/taskcluster/tasks/builds/b2g_dolphin_512_opt.yml
@@ +3,5 @@
> +task:
> +  scopes:
> +    - 'docker-worker:cache:build-dolphin-512-opt'
> +  metadata:
> +    name: B2G Dolphin 512 Opt

Can you prefix name with [TC] ( makes it clear to sheriffs / etc... what built this thing )

::: testing/taskcluster/tasks/builds/b2g_dolphin_base.yml
@@ +10,5 @@
> +      REPO_TRACE: 1
> +      VARIANT: user
> +      DEBUG: 0
> +
> +    # Emulators can take a very long time to build!

nit: copy/paste you can probably lower this value too ?
Attachment #8585124 - Flags: review?(jlal) → review+
r+ with few small comments ( feel free to land on BI once those are addressed )
James, Now version of quay.io/mozilla/builder is 0.5.4, so I think this file also need to be updated accordingly. Would you mind review this patch? Thanks!
Attachment #8581207 - Attachment is obsolete: true
Attachment #8585331 - Flags: review?(jlal)
Attachment #8585331 - Flags: review?(jlal) → review?(wcosta)
Comment on attachment 8585331 [details] [diff] [review]
Update builder version in phone-builder

Review of attachment 8585331 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/docker/phone-builder/Dockerfile
@@ -1,1 @@
> -FROM          quay.io/mozilla/builder:0.5.3

Usually, even when I only upgrade the base image version, I also upgrade the inherited image version. This makes easier to trace problems caused by side effects of the new image.
Do you mean I should also update phone-builder version from 0.0.11 to 0.0.12 in 'testing/docker/phone-builder/VERSION'?
Flags: needinfo?(wcosta)
(In reply to Kai-Zhen Li [:kli][:seinlin] from comment #15)
> Do you mean I should also update phone-builder version from 0.0.11 to 0.0.12
> in 'testing/docker/phone-builder/VERSION'?

Yep :)
Flags: needinfo?(wcosta)
https://hg.mozilla.org/mozilla-central/rev/ee5a8ba64485
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
reverted https://hg.mozilla.org/integration/b2g-inbound/rev/6bc7b1b0ad81
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment on attachment 8585331 [details] [diff] [review]
Update builder version in phone-builder

Now patch in b2g-inbound was backed out [1]. There will be a new full patch which also includes this version update.

https://hg.mozilla.org/integration/b2g-inbound/rev/6bc7b1b0ad81
Attachment #8585331 - Attachment is obsolete: true
Attachment #8585331 - Flags: review?(wcosta)
Target Milestone: mozilla39 → ---
Hi Wander, other than the part James've reviewed, I also update phone-builder's Dockerfile and VERSION in new patch. Would you mind review this? Thanks!
Attachment #8585124 - Attachment is obsolete: true
Attachment #8586159 - Flags: review?(wcosta)
Comment on attachment 8586159 [details] [diff] [review]
bug-1144463-add-dolphin_512-n-update-dolphin-2.patch

Review of attachment 8586159 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/taskcluster/scripts/phone-builder/build-dolphin.sh
@@ +38,5 @@
> +debug_flag=""
> +if [ 0$B2G_DEBUG -ne 0 ]; then
> +  debug_flag='--debug'
> +fi
> +

Instead of all the above stuff, you should use pre-build.sh script.

@@ +62,5 @@
> +mv $WORKSPACE/B2G/upload/b2g-*.android-arm.tar.gz $HOME/artifacts/b2g-android-arm.tar.gz
> +mv $WORKSPACE/B2G/upload/${TARGET}.zip $HOME/artifacts/${TARGET}.zip
> +mv $WORKSPACE/B2G/upload/gaia.zip $HOME/artifacts/gaia.zip
> +
> +ccache -s

If I see correctly, the only difference to build-phone.sh is the language file, right? If that's the case I would more inclined to patch tc to allow customize the language file path than duplicating the script. But this is up to you, we already have a bunch of duplicate code in the scripts and sooner or later we will have to fix that. If you think it is better having a separate script, I am not against it.

::: testing/taskcluster/tasks/builds/b2g_dolphin_512_eng.yml
@@ +15,5 @@
> +        platform: b2g-device-image
> +
> +  payload:
> +    cache:
> +      build-dolphin-512-eng: /home/worker/object-folder

s/object-folder/workspace

@@ +19,5 @@
> +      build-dolphin-512-eng: /home/worker/object-folder
> +    env:
> +      TARGET: 'dolphin-512'
> +      VARIANT: eng
> +

I see no reference to dolphin.zip, is it ok?

::: testing/taskcluster/tasks/builds/b2g_dolphin_512_opt.yml
@@ +14,5 @@
> +        platform: b2g-device-image
> +
> +  payload:
> +    cache:
> +      build-dolphin-512-opt: /home/worker/object-folder

s/object-folder/workspace

::: testing/taskcluster/tasks/builds/b2g_dolphin_eng.yml
@@ -8,3 @@
>    payload:
>      cache:
> -      build-hamachi-eng: /home/worker/object-folder

s/object-folder/workspace

::: testing/taskcluster/tasks/builds/b2g_dolphin_opt.yml
@@ +14,5 @@
> +        platform: b2g-device-image
> +
> +  payload:
> +    cache:
> +      build-dolphin-opt: /home/worker/object-folder

s/object-folder/workspace
Wcosta, I update the patch as you suggested. Dolphin build do not need dolphin.zip, as all blob can be built from source tree. I also push the new patch to try [1] to make sure it doesn't break other build.

[1] https://treeherder.allizom.org/#/jobs?repo=try&revision=6313e82a9ec8
Attachment #8586159 - Attachment is obsolete: true
Attachment #8586159 - Flags: review?(wcosta)
Attachment #8587497 - Flags: review?(wcosta)
Comment on attachment 8587497 [details] [diff] [review]
bug-1144463-add-dolphin_512-n-update-dolphin-3.patch

Review of attachment 8587497 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm
Attachment #8587497 - Flags: review?(wcosta) → review+
Wcosta, Thanks! Before push to inbound, could you tell me how should I build and push phone-builder 0.0.12 to quay?
Flags: needinfo?(wcosta)
To build phone-builder, you need to pass the S3 credentials to it. Unfortunately making this easy to get is still on our task list. Basically, you need to type this:

./build.sh phone-builder -i <aws access token> -k <aws secret key>

If you need an image built for tests, I can build it for you.
Flags: needinfo?(wcosta)
Unfortunately, I didn't have aws token and key. Could you build one and push to quay? I'd like to make sure my patch doesn't break any build on TC. Thanks!
Flags: needinfo?(wcosta)
I did want to point out that we didn't need to use the phone builder for dolphin since it does not use any magic bits from our partners (just builder would work)
Ops, forgot my last comment... So, I pushed phone-buider 0.0.12 to quay.io. Could you please push b2g-build and builder images too?
(In reply to James Lal [:lightsofapollo] from comment #27)
> I did want to point out that we didn't need to use the phone builder for
> dolphin since it does not use any magic bits from our partners (just builder
> would work)

Anyway, I think it is a good idea to use phone-builder to build dolphin, even if it is not necessary. It is intuitive for newcomers infer that phone images are built by the phone-builder docker.
Wcosta, Thanks! I push to try again, now all builds seem happy on TC [1]. I also try to use builder and it also does work properly. When I notice that there is a phone-builder, I think it could be intuitive to build a phone images by phone-builder.

[1] https://treeherder.allizom.org/#/jobs?repo=try&revision=c4ec5ada2818
https://hg.mozilla.org/mozilla-central/rev/2727e5aa21cd
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Wcosta, I fixed the execute permission of build-dolphin and update the path in dolphin config file. Would you mind have a look to this patch? Thanks!
Attachment #8596641 - Flags: review?(wcosta)
Comment on attachment 8596641 [details] [diff] [review]
bug-1144463-fix-2.patch

Review of attachment 8596641 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm
Attachment #8596641 - Flags: review?(wcosta) → review+
Wcosta, Thanks! I'll land it when try result are green.

https://treeherder.allizom.org/#/jobs?repo=try&revision=bd81f0dcd654
https://hg.mozilla.org/mozilla-central/rev/31cdda5d73e0
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Component: TaskCluster → General
Product: Testing → Taskcluster
Target Milestone: mozilla40 → mozilla41
Version: unspecified → Trunk
Resetting Version and Target Milestone that accidentally got changed...
Target Milestone: mozilla41 → ---
Version: Trunk → unspecified
You need to log in before you can comment on or make changes to this bug.