Closed Bug 1472236 Opened 6 years ago Closed 6 years ago

Add grcov to the toolchain by using fetch tasks

Categories

(Testing :: Code Coverage, enhancement)

Product:
Testing
Component:
Code Coverage
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(firefox63 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla63
Tracking Flags:
Tracking Status
firefox63 --- fixed

People

(Reporter: gabriel-v, Assigned: gabriel-v)

References

Details

Attachments

(1 file)

Bug 1472236 - Add grcov code coverage builds by using a fetch task.
59 bytes, text/x-review-board-request
marco
: review+
dustin
: review+
Details 
Add grcov to taskcluster/ci/fetch/toolchain.yml and stop downloading it from tooltool.

This will facilitate:

- easier releases of grcov (removes the manual releases to tooltool)
- simplifying the codecoverage.py script, because it won't have to download grcov
Assignee: nobody → tvijiala
Version: Version 3 → unspecified
Depends on: 1472243
Status: NEW → ASSIGNED
Summary: Add grcov to the toolchain → Add grcov to the toolchain by using fetch tasks
All the other fetch tasks pull only source code. We're planning to store binary releases on it. Is that a good idea?

Also, how does one fill out the `gpg-signature` field? Is it required?
Flags: needinfo?(gps)
The gpg-signature field is not required. That's just additional security protections for downloading external content that happens to be GPG signed.

Anyway, fetch tasks are intended to be used for "caching" dependencies as taskcluster artifacts. It can logically be used as a replacement for tooltool. Although the content to be fetched still needs to be canonically fetched from somewhere, so it isn't exactly the same.

There's nothing preventing you from using binary files with the fetch tasks. But for transparency in our build pipeline, it is highly recommended to fetch source, build it, and use the binary artifacts of that. Code coverage isn't in the critical path of shipping Firefox, so the binary dependency is OK (we don't want opaque binary bits influencing the Firefox we ship to users).
Flags: needinfo?(gps)
Progress is as follows:

- changed the "test" task kind to depend on "fetch" tasks
- test code coverage transform now adds grcov to "fetches"
- codecoverage.py now downloads and extracts the grcov release (using tarfile)

I tried using 'taskcluster/scripts/misc/fetch-content' to get the artifacts, but it requires python3. The script is non-trivial to switch to python2, as it uses concurrent.futures.ThreadPoolExecutor, pathlib.Path and so on.

Python3 is not installed on OSX (Bug 1466535) or Windows (Bug 1451107) machines.
For now, let's implement the downloading manually. Then, we will switch to using fetch-content when bug 1466535 and bug 1451107 are fixed.
Blocks: 1473541
See Also: → 1468812
Comment on attachment 8989979 [details]
Bug 1472236 - Add grcov code coverage builds by using a fetch task.

https://reviewboard.mozilla.org/r/255000/#review261838

LGTM for the code coverage parts! This will make the process of updating grcov quite shorter \o/

::: taskcluster/ci/fetch/toolchains.yml:198
(Diff revision 1)
>      gpg-signature:
>        sig-url: "{url}.asc"
>        key-path: build/unix/build-gcc/07F3DBBECC1A39605078094D980C197698C3739D.key
> +
> +grcov-linux-x86_64:
> +  description: grcov 0.2.1 binary release

Nit: let's remove the version from the description, so we only have one line to update when we update grcov
Attachment #8989979 - Flags: review?(mcastelluccio) → review+
Comment on attachment 8989979 [details]
Bug 1472236 - Add grcov code coverage builds by using a fetch task.

https://reviewboard.mozilla.org/r/255000/#review261842

::: testing/mozharness/mozharness/mozilla/testing/codecoverage.py:115
(Diff revision 1)
> -        # Create the grcov directory, get the tooltool manifest, and finally
> -        # download and unpack the grcov binary.
> +        # Create the grcov directory, then download it.
> +        # TODO: use the fetch-content script to download artifacts.
>          self.grcov_dir = tempfile.mkdtemp()
> +        ARTIFACT_URL = 'https://queue.taskcluster.net/v1/task/{task}/artifacts/{artifact}'
> +        for word in os.getenv('MOZ_FETCHES').split():
> +            artifact, task = word.split('@', 1)
> +            filename = os.path.basename(artifact)
> +            url = ARTIFACT_URL.format(artifact=artifact, task=task)
> +            self.download_file(url, parent_dir=self.grcov_dir)
>  
> -        if mozinfo.os == 'linux':
> +            with tarfile.open(os.path.join(self.grcov_dir, filename), 'r') as tar:

It would be nice if we could use the `fetch-content` script. For most workers you can look at how we install the `run-task` script and do the same thing with `fetch-content`.

::: testing/mozharness/mozharness/mozilla/testing/codecoverage.py:118
(Diff revision 1)
> +        ARTIFACT_URL = 'https://queue.taskcluster.net/v1/task/{task}/artifacts/{artifact}'
> +        for word in os.getenv('MOZ_FETCHES').split():
> +            artifact, task = word.split('@', 1)
> +            filename = os.path.basename(artifact)
> +            url = ARTIFACT_URL.format(artifact=artifact, task=task)
> +            self.download_file(url, parent_dir=self.grcov_dir)

I won't make this an issue, but it would also be nice if this lived in some shared location that could be re-used by any mozharness script/library.

I'd eventually like all mozharness artifact downloading to use `MOZ_FETCHES` and `fetch-content`.
(In reply to Andrew Halberstadt [:ahal] from comment #7)
> It would be nice if we could use the `fetch-content` script. For most
> workers you can look at how we install the `run-task` script and do the same
> thing with `fetch-content`.

Gabriel filed bug 1473541 for this to be done as a follow-up, depends on being able to use Python 3 on the test machines (as the script is only compatible with Python 3).

> 
> ::: testing/mozharness/mozharness/mozilla/testing/codecoverage.py:118
> (Diff revision 1)
> > +        ARTIFACT_URL = 'https://queue.taskcluster.net/v1/task/{task}/artifacts/{artifact}'
> > +        for word in os.getenv('MOZ_FETCHES').split():
> > +            artifact, task = word.split('@', 1)
> > +            filename = os.path.basename(artifact)
> > +            url = ARTIFACT_URL.format(artifact=artifact, task=task)
> > +            self.download_file(url, parent_dir=self.grcov_dir)
> 
> I won't make this an issue, but it would also be nice if this lived in some
> shared location that could be re-used by any mozharness script/library.
> 
> I'd eventually like all mozharness artifact downloading to use `MOZ_FETCHES`
> and `fetch-content`.

Where? Maybe testing/mozharness/mozharness/base/script.py? I see we have several utility functions there, such as 'download_file' (https://dxr.mozilla.org/mozilla-central/rev/6c0fa9a675c91390ca27664ffb626c56e8afea4d/testing/mozharness/mozharness/base/script.py#744).
Comment on attachment 8989979 [details]
Bug 1472236 - Add grcov code coverage builds by using a fetch task.

https://reviewboard.mozilla.org/r/255000/#review261842

> It would be nice if we could use the `fetch-content` script. For most workers you can look at how we install the `run-task` script and do the same thing with `fetch-content`.

Marco mentioned that fetch-content requires python 3 and python 3 still isn't installed everwhere :(. We should really fix that, but not worth blocking on it.
(In reply to Marco Castelluccio [:marco] from comment #9)
> > ::: testing/mozharness/mozharness/mozilla/testing/codecoverage.py:118
> > (Diff revision 1)
> > > +        ARTIFACT_URL = 'https://queue.taskcluster.net/v1/task/{task}/artifacts/{artifact}'
> > > +        for word in os.getenv('MOZ_FETCHES').split():
> > > +            artifact, task = word.split('@', 1)
> > > +            filename = os.path.basename(artifact)
> > > +            url = ARTIFACT_URL.format(artifact=artifact, task=task)
> > > +            self.download_file(url, parent_dir=self.grcov_dir)
> > 
> > I won't make this an issue, but it would also be nice if this lived in some
> > shared location that could be re-used by any mozharness script/library.
> > 
> > I'd eventually like all mozharness artifact downloading to use `MOZ_FETCHES`
> > and `fetch-content`.
> 
> Where? Maybe testing/mozharness/mozharness/base/script.py? I see we have
> several utility functions there, such as 'download_file'
> (https://dxr.mozilla.org/mozilla-central/rev/
> 6c0fa9a675c91390ca27664ffb626c56e8afea4d/testing/mozharness/mozharness/base/
> script.py#744).

Agreed with :ahal that we will figure this out later, as part of bug 1473541.
Attachment #8989979 - Flags: review?(dustin)
Comment on attachment 8989979 [details]
Bug 1472236 - Add grcov code coverage builds by using a fetch task.

https://reviewboard.mozilla.org/r/255000/#review262354

This looks mostly good. But the copying of environment variables in the low-level job code doesn't feel right to me. That's the main thing holding up r+ on my end. I've asked Dustin for review.

::: taskcluster/taskgraph/transforms/job/__init__.py:173
(Diff revision 2)
> +        # get all environment variables from the job, without overwriting the task's variables
> +        jobenv = job.get('worker', {}).get('env', {})
> +        taskenv = taskdesc.get('worker', {}).get('env', {})
> +        for key, value in jobenv.iteritems():
> +            taskenv.setdefault(key, value)

This is the transform that runs for all tasks.

I don't feel comfortable signing off on this. It feels like this should be done elsewhere. But I'm not sure where. Please ask Dustin for a 2nd opinion.

::: testing/mozharness/mozharness/mozilla/testing/codecoverage.py:123
(Diff revision 2)
> +        ARTIFACT_URL = 'https://queue.taskcluster.net/v1/task/{task}/artifacts/{artifact}'
> +        for word in os.getenv('MOZ_FETCHES').split():
> +            artifact, task = word.split('@', 1)
> +            filename = os.path.basename(artifact)
> +            url = ARTIFACT_URL.format(artifact=artifact, task=task)
> +            self.download_file(url, parent_dir=self.grcov_dir)

Ideally this would perform SHA256 verification. But that's not super important since this task isn't in the critical path of delivering bits that run on end-user machines (AFAIK).

We'll shake it out once we nuke mozharness and integrate MOZ_FETCHES into things more formally.
Attachment #8989979 - Flags: review?(gps)
Comment on attachment 8989979 [details]
Bug 1472236 - Add grcov code coverage builds by using a fetch task.

https://reviewboard.mozilla.org/r/255000/#review262354

> This is the transform that runs for all tasks.
> 
> I don't feel comfortable signing off on this. It feels like this should be done elsewhere. But I'm not sure where. Please ask Dustin for a 2nd opinion.

If I print the task and environment variable before the 'taskenv.setdefault(key, value)', I get the following list: https://pastebin.mozilla.org/9089353. 

Patch used to print these: https://pastebin.mozilla.org/9089280.

The only environment variable that was missing is MOZ_FETCHES, and it only happened for the code coverage test suites. Tracked it down to taskcluster/taskgraph/transforms/job/mozharness_test.py in function mozharness_test_on_docker, which overwrites the worker's env.

Updating patch to make mozharness_test_on_docker keep existing envs.

> Ideally this would perform SHA256 verification. But that's not super important since this task isn't in the critical path of delivering bits that run on end-user machines (AFAIK).
> 
> We'll shake it out once we nuke mozharness and integrate MOZ_FETCHES into things more formally.

The alternative here (used in the taskcluster/ci/misc/build_* scripts) is 'fetch-content task-artifacts $MOZ_FETCHES', which does not verify sha256, size or sig.

The sha256, size and pgp verification happens on the fetch tasks themselves.
Attachment #8989979 - Flags: review?(gps) → review?(dustin)
Comment on attachment 8989979 [details]
Bug 1472236 - Add grcov code coverage builds by using a fetch task.

https://reviewboard.mozilla.org/r/255000/#review262544

::: taskcluster/taskgraph/transforms/tests.py:728
(Diff revision 4)
>              test.pop('when', None)
>              test['optimization'] = None
>  
> +            # Add a fetch task for the grcov binary.
> +            if 'linux' in test['build-platform']:
> +                test['fetches'] = ['grcov-linux-x86_64']

For future expansion, this field should be added to the schema at the top of this file, and the same `setdefault`/`append` logic as you applied for `env` should be applied here.  Then other test transforms can set up fetches, or they can even be specified directly in a `.yml` file.
Attachment #8989979 - Flags: review?(dustin) → review+
Keywords: checkin-needed
Could you update the version number to 0.2.3? I've released it this morning (and hopefully it was my last manual release).
Keywords: checkin-needed
Depends on: 1474342
Depends on: 1474343
Blocks: 1474342, 1474343
No longer depends on: 1474342, 1474343
Keywords: checkin-needed
Blocks: 1473610
There are still 2 open issues that need to be fixed in order to land this patch.
Flags: needinfo?(tvijiala)
Keywords: checkin-needed
Comment on attachment 8989979 [details]
Bug 1472236 - Add grcov code coverage builds by using a fetch task.

https://reviewboard.mozilla.org/r/255000/#review262544

> For future expansion, this field should be added to the schema at the top of this file, and the same `setdefault`/`append` logic as you applied for `env` should be applied here.  Then other test transforms can set up fetches, or they can even be specified directly in a `.yml` file.

Future expansion tracked in bug 1474343.
Pushed by mcastelluccio@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a0976c8fca74
Add grcov code coverage builds by using a fetch task. r=dustin,marco
Issues resolved.
Flags: needinfo?(tvijiala)
Blocks: 1474575
https://hg.mozilla.org/mozilla-central/rev/a0976c8fca74
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: