Closed Bug 1336559 Opened 3 years ago Closed 3 years ago

Ability to depend on a build artifact in "job" tasks

Categories

(Firefox Build System :: Task Configuration, task)

task
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla54

People

(Reporter: ahal, Assigned: ahal)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

We are getting closer to being able to easily put python-unittests into their own separate tasks. However, some python-unittests (and other "source-check" tasks), need a firefox binary. For example, mozrunner tests, xpcshell/mochitest selftests, etc.

I have a WIP patch series that does the following:

1. Rename 'source-check' to 'source-test'. The name 'source-check' is a bit misleading if some of the tasks in there need a build, and it didn't seem worthwhile to pull those two things into separate kinds.

2. Add a 'require-build' boolean config option to using.run_task.

3. Add a 'build-labels' dict to the source-test/kind.yml config. The job transforms will use this dict to map test platforms to build platforms, e.g it might look like:
build-labels:
  linux64.*: build-linux64/opt

4. In the job transforms, set up the 'dependencies' key based on the build-labels and job['platform'] configs.

5. Also in the job transforms, construct the build artifact URL and set it in a GECKO_INSTALLER_URL environment variable. It would be left up to the tasks to download and install this however makes sense.

Once I've tested/fleshed this out a bit more and bug 1320194 lands, I'll upload it for feedback/review.
No longer blocks: 1320194
Depends on: 1320194
That sounds like a functional design, but it crosses some abstraction boundaries in an awkward way:

 - run.using run_task expects config['build-labels'], but that's not defined in every kind
 - it'd be reasonable to expect this to work for other run.using values, since it has little to do with run-task.py, but it doesn't work
 - the `tests` kind could use this approach, but it doesn't, so we have two approaches to solving the same problem
 - typically the kind implementation is the thing that defines dependencies

From our irc conversation, I was adequately convinced that these tests-needing-a-binary don't belong in the test kind, but I do think that they are sufficiently different in character from things like mozlint that it is worthwhile to create two separate kinds.

To pick a probably-bad name, the new "installer-check" kind would differ from the source-check kind:
 - depends on the `build` kind
 - constructs dependencies similar to the test kind implementation perhaps using config in kind.yml to find the build task
 - injects the GECKO_INSTALLER_URL env var using similar configuration
   (alternately, this could create test['build-artifact-name'] and a transform could add that to the env)

I can work on implementing something like that, if you'd like.
I think that's a reasonable approach too. Some counter-points to consider though:

1. Doing this means we would split up python-tests.yml into two python-tests.yml which are defined almost identically. Imo the similarities between these tasks far outweigh the minor detail of whether a build binary is used or not.
2. source-check is already a misnomer, because the python-tests aren't really "checks". They are bonafide unittests that have different behaviour on different platforms.

Anyway, I guess this is pretty much just bikeshedding and I'm happy to concede to your wishes. But don't start implementing anything until I get a chance to upload my WIP (I'll aim to do that early next week). At the very least you can probably use it for inspiration.
Inspiration and collaboration, sounds great!

Maybe "source-tests" and "installer-tests"?

We can see how it turns out -- it's hunch vs. hunch right now.
reassign to or needinfo me when your WIP is up, please?
Assignee: nobody → ahalberstadt
(In reply to Dustin J. Mitchell [:dustin] from comment #3)
> Inspiration and collaboration, sounds great!
> 
> Maybe "source-tests" and "installer-tests"?
> 
> We can see how it turns out -- it's hunch vs. hunch right now.

I don't like the name installer-tests, because that implies they are testing the installer, which isn't necessarily the case. For example, I'd like to create some mochitest selftests. The point of these will be to test the mochitest harness, but in order to do that, we'll incidentally need a Firefox binary. This could technically be any Firefox binary, including a downloaded nightly build (though it would be best to use a build from the same changeset in case there are corresponding changes that landed both to the platform and mochitest harness at the same time).

Anyway, I'm just formatting the WIP and should have it up shortly.
I'll let you decide if you want to take over this bug and start fresh, or if you want to continue down this vein and leave review comments.

The mochitest task in the third commit is just for example purposes (it doesn't actually exist). The only thing differentiating it from the other python-test tasks is the 'require-build: true' config.
Flags: needinfo?(dustin)
What a difference it makes to discuss real code!  I'm quite a bit happier with this based on the review request than when we spoke.  I'm still a little uncomfortable with the run-task job implementation consulting kind config and setting the dependency.  Specific things that struck me:

 - kind config should be configuration for the kind, not for the transforms
   - it would be surprising to find that a task with a different run.using would not support the build dependency
 - the platform matching in `build-platforms` should match that used for keyed-by fields (by-test-platform, etc.)
 - the installer name should be configured along with the platforms, rather than substring-matching on labels

But as I think about how I might do that differently, I wonder if the cure is worse than the disease.  Or at least, if this runs into my design error of defining kinds with a single implementation class.

So I think next steps are either
 - put this up for review and fix a few minor issues I could identify, then land; or
 - wait for me to attempt to redraft (probably not this week - I'm behind)
which would you prefer?
Flags: needinfo?(dustin) → needinfo?(ahalberstadt)
I figured I was probably making it sound more complicated than it was ;).

There isn't a huge rush to get this done, so if you'd like to tackle it that's fine. But I have a bit of time to hack on it now, so I'm also happy to go that route. In the latter case are you suggesting that I simply flag you on the existing review and we start iterating from there?
Flags: needinfo?(ahalberstadt)
Attachment #8833978 - Flags: review?(dustin)
Attachment #8833979 - Flags: review?(dustin)
Attachment #8833980 - Flags: review?(dustin)
Comment on attachment 8833980 [details]
Bug 1336559 - Add ability to depend on build artifacts to 'run_task' based tasks,

https://reviewboard.mozilla.org/r/110086/#review114332
Attachment #8833980 - Flags: review?(dustin) → review+
Comment on attachment 8833979 [details]
Bug 1336559 - Refactor keyed-by matching algorithm into standalone utility function,

https://reviewboard.mozilla.org/r/110084/#review114296

::: taskcluster/ci/source-test/kind.yml:18
(Diff revision 1)
> +build-labels:
> +    linux64.*: build-linux64/opt

Let's name this `dependent-build-platforms`.  Also, its syntax should match that of the keyed_by stuff in transforms (maybe it's time to factor some of the code in `taskcluster/taskgraph/transforms/base.py` out into `taskcluster/taskgraph/util`).  It should also have some comments about what it does, and that it only works with the `mach` and `run-task` run.using variants.

::: taskcluster/taskgraph/transforms/job/__init__.py:154
(Diff revision 1)
> +        if 'mozbuild' in taskdesc['label']:
> +            import json
> +            print(json.dumps(taskdesc, indent=2))

Debugging output?

::: taskcluster/taskgraph/transforms/job/common.py:79
(Diff revision 1)
> +    if 'macosx' in label:
> +        target = 'target.dmg'
> +    elif 'android' in label:
> +        target = 'target.apk'
> +    else:
> +        target = 'target.tar.bz2'

Is this stable enough that it can be left hidden in code here, or should it be some kind of configuration?

::: taskcluster/taskgraph/transforms/job/common.py:89
(Diff revision 1)
> +        target = 'target.tar.bz2'
> +    build_artifact = 'public/build/{}'.format(target)
> +    installer_url = ARTIFACT_URL.format('<build>', build_artifact)
> +
> +    env = taskdesc['worker'].setdefault('env', {})
> +    env.update({'GECKO_INSTALLER_URL': {'task-reference': installer_url}})

The comments suggest that `run-task` will interpret this env var, but I don't see the variable referenced in that file (`taskcluster/docker/recipes/run-task`).  Does something else do so?

::: taskcluster/taskgraph/transforms/job/mach.py:19
(Diff revision 1)
> +
> +    # Whether the job requires a build artifact or not. If True, the task
> +    # will depend on a build task and run-task will download and set up the
> +    # installer.

Please mention that the build is discovered using the mapping in kind.yml, based on the job platform.

::: taskcluster/taskgraph/transforms/job/run_task.py:34
(Diff revision 1)
> +    # Whether the job requires a build artifact or not. If True, the task
> +    # will depend on a build task and run-task will download and set up the
> +    # installer.

Same comment update here

::: taskcluster/taskgraph/transforms/job/run_task.py:49
(Diff revision 1)
>  
>      worker = taskdesc['worker'] = copy.deepcopy(job['worker'])
>  
>      if run['checkout']:
>          docker_worker_support_vcs_checkout(config, job, taskdesc)
> +    

stray whitespace
Attachment #8833979 - Flags: review?(dustin) → review-
Comment on attachment 8833978 [details]
Bug 1336559 - Rename source-check kind to source-test,

https://reviewboard.mozilla.org/r/110082/#review114334
Attachment #8833978 - Flags: review?(dustin) → review+
Comment on attachment 8833979 [details]
Bug 1336559 - Refactor keyed-by matching algorithm into standalone utility function,

https://reviewboard.mozilla.org/r/110084/#review114296

> Is this stable enough that it can be left hidden in code here, or should it be some kind of configuration?

I straight copied this from the test kind here:
https://dxr.mozilla.org/mozilla-central/source/taskcluster/taskgraph/transforms/tests.py#358

It should probably live in configuration somewhere though, but not sure where.

> The comments suggest that `run-task` will interpret this env var, but I don't see the variable referenced in that file (`taskcluster/docker/recipes/run-task`).  Does something else do so?

This was copied from the test kind, and I got it working through trial and error. I'm not 100% sure how this works, though there is a mention of it in the docs, so I assume it's a general thing?
http://gecko.readthedocs.io/en/latest/taskcluster/taskcluster/taskgraph.html?highlight=task-reference#task-parameterization
Comment on attachment 8833979 [details]
Bug 1336559 - Refactor keyed-by matching algorithm into standalone utility function,

https://reviewboard.mozilla.org/r/110084/#review114978

The GECKO_INSTALLER_URL thing has me baffled -- I don't see it in testing/ or in taskcluster/ -- but at worst that means this patch doesn't actually download the installer, and you file a follow-on bug :)

::: taskcluster/taskgraph/util/attributes.py:47
(Diff revision 2)
>  
>  
> +def keymatch(attributes, target):
> +    """Determine if any keys in attributes are a regex match to target, then
> +    return the list of matching key/value pairs."""
> +    return [(k, v) for k, v in attributes.iteritems() if re.match(k + '$', target)]

I was hoping to see the matching behavior exact-match-first, then regexp, then default, as resolve_keyed_by does.  But if someone is someday surprised by the different behaviors, it's easy enough to add at that time.
Attachment #8833979 - Flags: review?(dustin) → review+
Comment on attachment 8833979 [details]
Bug 1336559 - Refactor keyed-by matching algorithm into standalone utility function,

https://reviewboard.mozilla.org/r/110084/#review114978

Yeah, the 3rd commit in this series was just an example usage.. I never meant to get you to review it, sorry! I'll be pruning it before landing this series, and follow up work will actually make use of 'require-build' in another bug.

> I was hoping to see the matching behavior exact-match-first, then regexp, then default, as resolve_keyed_by does.  But if someone is someday surprised by the different behaviors, it's easy enough to add at that time.

I don't mind doing this now.
Blocks: 1342230
Comment on attachment 8833979 [details]
Bug 1336559 - Refactor keyed-by matching algorithm into standalone utility function,

I guess mozreview isn't great at pruning a commit, then adding a new one, then reshuffling them.

You *have* reviewed the 3rd commit (minus some minor interdiff changes), but you *haven't* reviewed the 2nd commit (this one). Since mozreview doesn't seem to let me reset the review flag, I'm resetting it here.
Attachment #8833979 - Flags: review+ → review?(dustin)
Comment on attachment 8833979 [details]
Bug 1336559 - Refactor keyed-by matching algorithm into standalone utility function,

https://reviewboard.mozilla.org/r/110084/#review116766

/me makes giddy clappy noises

Thanks :)
Attachment #8833979 - Flags: review?(dustin) → review+
Pushed by ahalberstadt@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ed46199e1f44
Rename source-check kind to source-test, r=dustin
https://hg.mozilla.org/integration/autoland/rev/23fbbffc0727
Refactor keyed-by matching algorithm into standalone utility function, r=dustin
https://hg.mozilla.org/integration/autoland/rev/0ed19e152444
Add ability to depend on build artifacts to 'run_task' based tasks, r=dustin
https://hg.mozilla.org/mozilla-central/rev/ed46199e1f44
https://hg.mozilla.org/mozilla-central/rev/23fbbffc0727
https://hg.mozilla.org/mozilla-central/rev/0ed19e152444
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Product: TaskCluster → Firefox Build System
You need to log in before you can comment on or make changes to this bug.