Closed Bug 1213011 Opened 4 years ago Closed 4 years ago

Add a route for simulator post-build task

Categories

(Firefox Build System :: Task Configuration, task)

task
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla44

People

(Reporter: ochameau, Assigned: ochameau)

References

Details

Attachments

(3 files, 6 obsolete files)

That, to easily refer to lastest nighly build of it.
Attached patch patch v1 (obsolete) — Splinter Review
I matched the route name of mulet task.
I wish I could use a variable to describe the platform,
like {{platform}} or something alike that would be linux32,linux64,macos,win32,...
As this post-build task, may, in future be applied to more than just linux64 mulet build.

Also, is there anything I can do locally to test such change?
Like ./mach graph or run some test suite?
Attachment #8671551 - Flags: review?(garndt)
I imagine I also need that to offer a link to "latest" xpi via index.taskcluster.net.
Before that patch, the filename contains the build date and so is hard to predict.
With this patch the filename will be always the same.

Greg, Is that fine, if from now on, I ask review to :jryans for the build-simulator.sh
who help me maintaining this simulator?
Attachment #8671556 - Flags: review?(garndt)
Comment on attachment 8671551 [details] [diff] [review]
patch v1

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

So, routes could be whatever you want them to be, but I think the routes that are within mulet are old style routes....they have been since replaced by those found in tasks/build.yml .

You can run the `mach taskcluster-graph` command (need to pass in some arguments to it) and that will spit out some JSON that you can verify.  Or you could just push to try and have it kick off and see how it looks for you in the index browser.  I would recommend starting off with index routes with like "index.public.garbage.ochameu..." so you don't pollute any other namespaces.

If this is the route you actually want, then r+
Attachment #8671551 - Flags: review?(garndt) → review+
Comment on attachment 8671556 [details] [diff] [review]
Use stable file name for simulator xpi in TC.

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

Definitely nice to have a artifact name that remains consistent rather than one with version numbers.
Attachment #8671556 - Flags: review?(garndt) → review+
Comment on attachment 8671551 [details] [diff] [review]
patch v1

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

::: testing/taskcluster/tasks/post-builds/mulet_simulator.yml
@@ +17,5 @@
>    provisionerId: aws-provisioner-v1
>    schedulerId: task-graph-scheduler
>  
> +  routes:
> +    - 'index.buildbot.branches.{{project}}.linux64-simulator'

Why do we want this route when we're not using buildbot here?

The Mulet builds are under index.gecko.v2.*, shouldn't this be under the same tree?
index.gecko.v2.* is handled automagically and doesn't support post-build tasks from what I understood.
Whereas route specified here are manual thing, this is kind of workaround.
TBH I don't really care about the name as soon as we can have a link to latest build.
I just copy pasted the route of mulet build, which as you highlight looks wrong by itself.
I think taskcluster team is aware of this and just use v2 and automatic routes instead.
For now it seems that at least conforming to the v1 format would be good.

https://dxr.mozilla.org/mozilla-central/source/testing/taskcluster/tasks/build.yml#21
Attached patch patch v2Splinter Review
Now uses `index.gecko.v1.{{project}}.latest.simulator.{{build_type}}` as route.
But I need a tweak to mach_command.py to expose build_type to post-build yml files!
Attachment #8671551 - Attachment is obsolete: true
Attachment #8672604 - Flags: review+
This is the additional patch I would need for this route, using build_type.
Today, we are exposing build_{name,type,product} only to `routes` field.
That is misleading as you could easily thing it is also exposed in `command`
or other YML fields!
With this patch, these variables are now exposed to the whole YML,
as well as the post-build task.
But I see some differences in the routes with this patch,
that I can't explain.
For example, without this patch we get this route:
  queue:route:index.gecko.v1.mozilla-central.latest.linux.linux32.opt
whereas we get this with the new patch:
  queue:route:index.gecko.v1.mozilla-central.latest.linux.linux.opt
Attachment #8672607 - Flags: review?(garndt)
With all these patches, I can see the latest task easily via url like this:
  https://tools.taskcluster.net/index/#gecko.v1.try.latest.simulator/gecko.v1.try.latest.simulator.opt
And even get the xpi like this \o/
  https://index.taskcluster.net/v1/task/gecko.v1.try.latest.simulator.opt/artifacts/public/build/fxos-simulator-2.5.20151012130140-linux64.xpi
(I forgot to push the build-simulator.sh patch on try to get a unique xpi name)
Comment on attachment 8672607 [details] [diff] [review]
Allow post-build tasks to use build_{name,type,product} variables

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

Definitely need to troubleshoot why the routes changed before and after patch.  We can't have any mismatch in the routes that were being generated as these are heavily relied upon to be stable.

r- for now until we can verify what is causing the routes to be changed.  Routes before and after should be compared and resolved, and also probably should just link to a try push running everything just to make sure nothing is obviously broken.

::: testing/taskcluster/mach_commands.py
@@ +114,5 @@
>      :param dict task: task definition.
>      :param json_routes: the list of routes to use from routes.json
>      :param parameters: dictionary of parameters to use in route templates
>      """
>      fmt = parameters.copy()

Do we need to copy the parameters anymore if we are not updating them?
Attachment #8672607 - Flags: review?(garndt) → review-
So I figured out why the routes were different.
I think there was something wrong with decorate_task_json_routes.
It used to pull build_name/build_type from `build` (itself comes from job_flags.yml).
From the jobs flags 32bits linux is labelled "linux" (see testing/taskcluster/tasks/branches/base_jobs.yml),
whereas the build_name used in yml files is "linux32" (see testing/taskcluster/tasks/builds/linux32_clobber.yml).

I think we should have only one source of truth and use the variables set in build ymls.
(And may be ensure that job flags yml uses the same names?)

In order to do that, we have to somehow pull these build_name/build_type attributes
out of build ymls. This patch allows to do that.
The only side effect I see is that build_name/build_type/build_product attributes now appear
in tasckcluster-graph. On build objects that now have taskId, task, build_name, build_product and build_type attributes.
Attachment #8673105 - Flags: review?(garndt)
With the previous patch, this one now doesn't introduce any different behavior in tasckcluster-graph,
except for post-build tasks, that now have access to all these variables.
Attachment #8672607 - Attachment is obsolete: true
Attachment #8673106 - Flags: review?(garndt)
so with these two patches I am unable to generate a graph like I would have done before (and like how the decision task does it).

Also, the tests for test_templates.py are now failing.

$ mach taskcluster-graph --project try  --message "try: -b do -p all -u all" --head-repository http://hg.mozilla.org/integration/b2g-inbound --head-rev tip --owner garndt@mozilla.com
Error running mach:

    ['taskcluster-graph', '--project', 'try', '--message', 'try: -b do -p all -u all', '--head-repository', 'http://hg.mozilla.org/integration/b2g-inbound', '--head-rev', 'tip', '--owner', 'garndt@mozilla.com']

The error occurred in the implementation of the invoked mach command.

This should never occur and is likely a bug in the implementation of that
command. Consider filing a bug for this issue.

If filing a bug, please include the full output of mach, including this error
message.

The details of the failure are as follows:

KeyError: 'build_name'

  File "/Users/mozilla/work/test_gecko/testing/taskcluster/mach_commands.py", line 371, in create_graph
    build_parameters['build_name'] = build_task['build_name']


--------------------------------------------

$ mach python-test tests/test_templates.py
 0:00.37 /Users/mozilla/work/test_gecko/obj-x86_64-apple-darwin13.4.0/_virtualenv/bin/python ./tests/test_templates.py
 0:00.61 TEST-PASS | ./tests/test_templates.py | test_deep_inheritance
 0:00.62 TEST-UNEXPECTED-FAIL | ./tests/test_templates.py | line 44, test_inheritance: {'content': 'content', 'variable': 'inherit', 'woot': 'inherit'} != {'content': 'content', 'variable': 'inherit'}
 0:00.62 - {'content': 'content', 'variable': 'inherit', 'woot': 'inherit'}
 0:00.62 ?                                    -------------------
 0:00.62
 0:00.62 + {'content': 'content', 'variable': 'inherit'}
 0:00.62 TEST-PASS | ./tests/test_templates.py | test_inheritance_circular
 0:00.63 TEST-UNEXPECTED-FAIL | ./tests/test_templates.py | line 55, test_inheritance_implicat_pass: {'a': 'a', 'c': 'c', 'b': 'b', 'values': ['overriden', 'b', 'c']} != {'values': ['overriden', 'b', 'c']}
 0:00.63 - {'a': 'a', 'b': 'b', 'c': 'c', 'values': ['overriden', 'b', 'c']}
 0:00.63 + {'values': ['overriden', 'b', 'c']}
 0:00.63 TEST-PASS | ./tests/test_templates.py | test_inheritance_with_simple_extensions
 0:00.63 TEST-PASS | ./tests/test_templates.py | test_invalid_path
 0:00.63 TEST-PASS | ./tests/test_templates.py | test_no_templates
 0:00.63 TEST-PASS | ./tests/test_templates.py | test_with_templates
Comment on attachment 8673106 [details] [diff] [review]
Allow post-build tasks to use build_{name,type,product} variables v2

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

seems that a graph can't be generated now and tests fail
Attachment #8673106 - Flags: review?(garndt) → review-
New patch, still good regarding just the simulators:
  https://tools.taskcluster.net/task-graph-inspector/#NgVnjyusSnGshHOopkVrlQ/W-UL5_bLRimtrCkGIiNg6g/0

Is there any specific try run I should spawn or do I need to do all/all ?!
Assignee: nobody → poirot.alex
I haven't been able to run testing/taskcluster/tests/test_templates.py.
mach python-test fails with python exception, not being able to find TC modules...

I don't know if that's the right behavior...
One alternative could be to set build_name/build_type from the inherited variables
in build.yml and build_phone.yml.
Attachment #8673105 - Attachment is obsolete: true
Attachment #8673105 - Flags: review?(garndt)
Attachment #8676921 - Flags: review?(garndt)
Dolphin builds ends up with undefined build_name/build_type,
so the routes set by phone_build.yml don't work.
Isn't that an issue?

In any case I made it so that we would accept a build stack without these attributes.
Attachment #8673106 - Attachment is obsolete: true
Attachment #8676923 - Flags: review?(garndt)
I was looking over this and it seems that now we have things in the graph we didn't have before and are not valid properties for the graph... build_name, build_product, and build_type now show up in the final graph when these are just variables used for rendering the templates.
> Dolphin builds ends up with undefined build_name/build_type,
> so the routes set by phone_build.yml don't work.
> Isn't that an issue?

That does seem like an issue, perhaps :wcosta knows more.  

Wander, it appears in the graph as:
"index.gecko.v1.try.revision.linux.tip.."
Flags: needinfo?(wcosta)
(In reply to Greg Arndt [:garndt] from comment #23)
> > Dolphin builds ends up with undefined build_name/build_type,
> > so the routes set by phone_build.yml don't work.
> > Isn't that an issue?
> 
> That does seem like an issue, perhaps :wcosta knows more.  
> 
> Wander, it appears in the graph as:
> "index.gecko.v1.try.revision.linux.tip.."

dolphin tasks don't setup these variables, because of that they are undefined. This is problem a mistake made by the original task author that passed by the reviewer's eyes.
Flags: needinfo?(wcosta)
Keywords: leave-open
Ok, then here is another approach, already suggested in comment 20.
Instead of hacking into template behavior, 
I define build_type/build_name like build_product, from build.yml and phone_build.yml.
Then I can easily fetch that from mach_commands.py.

I keep throwing patches at you, but it could be more efficient
if you provide more feedback and sometimes highlights what
I should or could be implementing instead of only highlighting what artifacts
cause my patches.
Attachment #8679003 - Flags: review?(garndt)
Attachment #8676921 - Attachment is obsolete: true
Attachment #8676923 - Attachment is obsolete: true
Attachment #8676921 - Flags: review?(garndt)
Attachment #8676923 - Flags: review?(garndt)
(In reply to Alexandre Poirot [:ochameau] from comment #26)
> Created attachment 8679003 [details] [diff] [review]
> Allow post-build tasks to use build_{name,type,product} variables v4
> 
> Ok, then here is another approach, already suggested in comment 20.
> Instead of hacking into template behavior, 
> I define build_type/build_name like build_product, from build.yml and
> phone_build.yml.
> Then I can easily fetch that from mach_commands.py.
> 
> I keep throwing patches at you, but it could be more efficient
> if you provide more feedback and sometimes highlights what
> I should or could be implementing instead of only highlighting what artifacts
> cause my patches.

That's definitely fair feedback, and I certainly didn't mean to cause an issue so I apologize.  I will be taking a look at this patch in the morning.
Comment on attachment 8679003 [details] [diff] [review]
Allow post-build tasks to use build_{name,type,product} variables v4

Reviewed the before and after output and verified only the route for simulator was changed (based on the previous patch that added the route to it).  Route now contains the 'opt' build_type
Attachment #8679003 - Flags: review?(garndt) → review+
https://hg.mozilla.org/integration/fx-team/rev/778030b1b88b7857d296eff8d63850c701c35139
Bug 1213011 - Allow post-build tasks to use build_{name,type,product} variables. r=garndt

https://hg.mozilla.org/integration/fx-team/rev/d41522b5aed48941893f47d57c54e9cab6e5b5d1
Bug 1213011 - Add a route to post-build simulator task to easily download it. r=garndt
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/778030b1b88b
https://hg.mozilla.org/mozilla-central/rev/d41522b5aed4
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Product: TaskCluster → Firefox Build System
You need to log in before you can comment on or make changes to this bug.