Closed Bug 1174376 Opened 9 years ago Closed 9 years ago

Enable Spidermonkey jobs via TaskCluster on Try

Categories

(Release Engineering :: General, defect)

Unspecified
Linux
defect
Not set
normal

Tracking

(firefox42 fixed)

RESOLVED FIXED
Tracking Status
firefox42 --- fixed

People

(Reporter: ffledgling, Assigned: ffledgling)

References

Details

Attachments

(1 file, 5 obsolete files)

After work on running Spidermonkey on try is done successfully, we should switch from using Buildbot to TaskCluster for these jobs
Assignee: nobody → ffledgling
Depends on: 1164656
This might be a little rough around the edges, but feedback would be great nonetheless.

Try push with current patch -- https://treeherder.mozilla.org/#/jobs?repo=try&revision=d023d3b08985

Try push with an earlier version (but similar), that succeeded, https://treeherder.mozilla.org/#/jobs?repo=try&revision=1d16f57a8fd3
Attachment #8625999 - Flags: feedback?(winter2718)
This is looking really good so far: a couple of comments. Let's use quay.io/mrrrgn/desktop-build -- I know it's my own account, but it's "official" for now until we decide on a official Mozilla docker repo.

Then, let's modify the treeherder symbol so that it's not just a 'B' -- The current jobs are: SM(cgc e r) 

To add that you can use:
  extra:
    treeherder:
      groupSymbol: SM-tc
      groupName: Spider Monkey, submitted by taskcluster
      symbol: p
      collection:
        debug: false
Comment on attachment 8625999 [details]
Initial Draft of patch to enable spidermonkey jobs on try

See the comment I just left. Looking good, just needs a few tweaks. :)
Attachment #8625999 - Flags: feedback?(winter2718)
Attached patch 2nd draft (obsolete) — Splinter Review
Adding updated patch based on mrrrgn's comments.
Full suite of all spidermonkey builds incoming soon.
Attachment #8625999 - Attachment is obsolete: true
Attached patch All variants (obsolete) — Splinter Review
Added all the Spidermonkey builds to try.

Working try push here -- https://treeherder.mozilla.org/#/jobs?repo=try&revision=595e62cc0cfa

NI'ing sfink for additional input.
Attachment #8626953 - Attachment is obsolete: true
Flags: needinfo?(sphink)
Attachment #8627413 - Flags: feedback?(winter2718)
Attachment #8627413 - Flags: feedback?(garndt)
RyanVM's already pointed out that the OSX jobs don't belong there at all. This goes back a question in https://bugzilla.mozilla.org/show_bug.cgi?id=1164656#c20 .

Other feedback is welcome as well.
Comment on attachment 8627413 [details] [diff] [review]
All variants

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

Looking good, just had some comments/questions.

::: testing/taskcluster/tasks/builds/sm_base.yml
@@ +5,5 @@
> +
> +  routes:
> +    - 'ffledgling.test.spidermonkey'
> +    #- 'index.buildbot.branches.{{project}}.sm-plain'
> +    #- 'index.buildbot.revisions.{{head_rev}}.{{project}}.sm-plain'

My guess would be that these will not be commented out once deployed.  Although I'm not sure if there is any risk not having them commented out them while testing.  They are just for try right now and the revision would be the one you're pushing.  Not sure if the index is used much for try so maybe I'm mistaken.

@@ +8,5 @@
> +    #- 'index.buildbot.branches.{{project}}.sm-plain'
> +    #- 'index.buildbot.revisions.{{head_rev}}.{{project}}.sm-plain'
> +
> +  scopes:
> +    - 'docker-worker:cache:build-linux64-workspace'

Is there value in sharing this cache with the other linux64 builds?  Since this cache contains not only the sources, but also objdirs, I'm not sure if SM can make use of that.

@@ +9,5 @@
> +    #- 'index.buildbot.revisions.{{head_rev}}.{{project}}.sm-plain'
> +
> +  scopes:
> +    - 'docker-worker:cache:build-linux64-workspace'
> +    # Do I need this?

whatever caches listed under payload.cache will require a scope for it otherwise the task will be rejected by the worker.  I'm not sure if spider monkey builds need tooltool, but might not and if that's the case you might not need the cache at all (thus removing scope and payload.cache.tooltool-cache).  It doesn't hurt having it though.

@@ +14,5 @@
> +    - 'docker-worker:cache:tooltool-cache'
> +
> +  payload:
> +    #image: '{{#docker_image}}desktop-build{{/docker_image}}'
> +    image: 'quay.io/ffledgling/build-desktop-temp'

I think I recall you saying that this was changed because the desktop-build image wasn't available.  We'll just want to make sure to change this back and make sure that image is freely available to everyone.

@@ +33,5 @@
> +    treeherderEnv:
> +      - staging
> +      - production
> +    treeherder:
> +      groupSymbol: SM-tc

Are spidermonkey builds currently reported to TH? If not, I'm not sure if we need to include "-tc".  From my understanding, including "-tc" was mainly for when we had buildbot and tc jobs living side-by-side.

@@ +34,5 @@
> +      - staging
> +      - production
> +    treeherder:
> +      groupSymbol: SM-tc
> +      groupName: Spider Monkey, submitted by taskcluster

is the ', submitted by taskcluster' here necessary?

@@ +36,5 @@
> +    treeherder:
> +      groupSymbol: SM-tc
> +      groupName: Spider Monkey, submitted by taskcluster
> +      collection:
> +        debug: false

If the purpose here is for this to appear ont he 'opt' line in tree herder, it does this by default if the collection key is not specified. https://dxr.mozilla.org/mozilla-central/source/testing/taskcluster/mach_commands.py#318
Attachment #8627413 - Flags: feedback?(garndt) → feedback+
Comment on attachment 8627413 [details] [diff] [review]
All variants

This is super close. Great work so far!
Attachment #8627413 - Flags: feedback?(winter2718)
(In reply to Greg Arndt [:garndt] from comment #7)
> Comment on attachment 8627413 [details] [diff] [review]
> All variants
> 
> Review of attachment 8627413 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looking good, just had some comments/questions.
> 
> ::: testing/taskcluster/tasks/builds/sm_base.yml
> @@ +5,5 @@
> > +
> > +  routes:
> > +    - 'ffledgling.test.spidermonkey'
> > +    #- 'index.buildbot.branches.{{project}}.sm-plain'
> > +    #- 'index.buildbot.revisions.{{head_rev}}.{{project}}.sm-plain'
> 
> My guess would be that these will not be commented out once deployed. 
> Although I'm not sure if there is any risk not having them commented out
> them while testing.  They are just for try right now and the revision would
> be the one you're pushing.  Not sure if the index is used much for try so
> maybe I'm mistaken.
> 
> @@ +8,5 @@
> > +    #- 'index.buildbot.branches.{{project}}.sm-plain'
> > +    #- 'index.buildbot.revisions.{{head_rev}}.{{project}}.sm-plain'
> > +
> > +  scopes:
> > +    - 'docker-worker:cache:build-linux64-workspace'
> 
> Is there value in sharing this cache with the other linux64 builds?  Since
> this cache contains not only the sources, but also objdirs, I'm not sure if
> SM can make use of that.
> 
> @@ +9,5 @@
> > +    #- 'index.buildbot.revisions.{{head_rev}}.{{project}}.sm-plain'
> > +
> > +  scopes:
> > +    - 'docker-worker:cache:build-linux64-workspace'
> > +    # Do I need this?
> 
> whatever caches listed under payload.cache will require a scope for it
> otherwise the task will be rejected by the worker.  I'm not sure if spider
> monkey builds need tooltool, but might not and if that's the case you might
> not need the cache at all (thus removing scope and
> payload.cache.tooltool-cache).  It doesn't hurt having it though.
> 
> @@ +14,5 @@
> > +    - 'docker-worker:cache:tooltool-cache'
> > +
> > +  payload:
> > +    #image: '{{#docker_image}}desktop-build{{/docker_image}}'
> > +    image: 'quay.io/ffledgling/build-desktop-temp'
> 
> I think I recall you saying that this was changed because the desktop-build
> image wasn't available.  We'll just want to make sure to change this back
> and make sure that image is freely available to everyone.

Yes, as soon as the patch in Bug 1164656 lands, this can be changed to the 'official' version.
> 
> @@ +33,5 @@
> > +    treeherderEnv:
> > +      - staging
> > +      - production
> > +    treeherder:
> > +      groupSymbol: SM-tc
> 
> Are spidermonkey builds currently reported to TH? If not, I'm not sure if we
> need to include "-tc".  From my understanding, including "-tc" was mainly
> for when we had buildbot and tc jobs living side-by-side.

They are. See https://treeherder.mozilla.org/help.html for the SM builds.

> @@ +34,5 @@
> > +      - staging
> > +      - production
> > +    treeherder:
> > +      groupSymbol: SM-tc
> > +      groupName: Spider Monkey, submitted by taskcluster
> 
> is the ', submitted by taskcluster' here necessary?

I'm not entirely sure, but I'm following the convention based on what I saw for the linux builds, doesn't really seem to hurt, since Spidermonkey builds exist on Treeherder outside of TaskCluster as well.

> @@ +36,5 @@
> > +    treeherder:
> > +      groupSymbol: SM-tc
> > +      groupName: Spider Monkey, submitted by taskcluster
> > +      collection:
> > +        debug: false
> 
> If the purpose here is for this to appear ont he 'opt' line in tree herder,
> it does this by default if the collection key is not specified.
> https://dxr.mozilla.org/mozilla-central/source/testing/taskcluster/
> mach_commands.py#318



Everything else, I've addressed in my updated patch.
hI've incorporated most of gardnt's suggestions in this patch, I've also cleaned up some other stuff like proper separation b/w debug and opt. The only fiddly bits left here are the artifact upload bits, which should be sorted out soon.

Note: This patch needs to land after the patch for Bug 11646656 lands.
Attachment #8627413 - Attachment is obsolete: true
Attached patch Spidermonkey-TC-try (obsolete) — Splinter Review
Final one, with all the issues sorted out.
This builds on-top of the patch from Bug 1164656, so in case you're testing locally, you might want to apply that one first.

Successful try push with these exact changes -- https://treeherder.mozilla.org/#/jobs?repo=try&revision=8a316a5d4314
Attachment #8628588 - Attachment is obsolete: true
Attachment #8629125 - Flags: review?(winter2718)
Attachment #8629125 - Flags: review?(winter2718) → review+
mrrrgn, This has the changes, and uses your repo -- https://treeherder.mozilla.org/#/jobs?repo=try&revision=2ace7278cff0

+ a bunch of other comments removed that I forgot to kill.
Attachment #8629125 - Attachment is obsolete: true
Attachment #8629213 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/cec27384d57d
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Flags: needinfo?(sphink)
Component: General Automation → General
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: