1473067 - Run Rust tests in coverage builds

Status: RESOLVED FIXED

Description

[Marco Castelluccio [:marco]]

We will need to run Rust tests in coverage builds.
This is a bit different than what we are used to, as the tests are run in the build itself and not in a test task, which means we will have to parse the gcda files on the build machine too.

Is enabling Rust tests just a matter of adding "ac_add_options --enable-rust-tests" to the ccov manifests?

Comment 1

[Marco Castelluccio [:marco]]

Attachment: Patch

Comment 2

[Marco Castelluccio [:marco]]

Comment on attachment 8997098 [details] [diff] [review]
Patch

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

I forgot to add the rusttests_coverage.py to the script, but it's not doing anything fancy (just some pre-operations for ccov, running the command, then some post-operations for ccov).

::: config/rules.mk
@@ +984,4 @@
>  
>  # Defining all of this for ASan/TSan builds results in crashes while running
>  # some crates's build scripts (!), so disable it for now.
> +ifndef MOZ_CODE_COVERAGE

Presumably this won't be needed and filtering out the '--coverage' flag is enough on automation.
Locally I see many other flags (unrelated to coverage) that make this fail.

Comment 3

[(not currently active) Ted Mielczarek]

Comment on attachment 8997098 [details] [diff] [review]
Patch

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

I think this should be fine. Obviously I'd like to see the Python script as well to actually grant r+.

::: config/rules.mk
@@ +984,4 @@
>  
>  # Defining all of this for ASan/TSan builds results in crashes while running
>  # some crates's build scripts (!), so disable it for now.
> +ifndef MOZ_CODE_COVERAGE

You might be OK, but note that if you try to do a cross-compile ccov build you'll almost certainly fail somewhere if you don't set the linker properly. If filtering out `--coverage` is enough then you should stick with that.

Comment 4

[Marco Castelluccio [:marco]]

Attachment: Patch

Comment 5

[Marco Castelluccio [:marco]]

Attachment: Patch

(Updated after some recent changes around the toolchain/fetch tasks)

Comment 6

[Marco Castelluccio [:marco]]

Comment on attachment 9012934 [details] [diff] [review]
Patch

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

::: testing/runrusttests.py
@@ +77,5 @@
> +        shutil.rmtree(gcov_dir)
> +
> +        grcov_zip_path = os.path.join(build_obj.topobjdir, 'code-coverage-grcov.zip')
> +        with zipfile.ZipFile(grcov_zip_path, 'a', zipfile.ZIP_DEFLATED) as z:
> +            index = max([0] + [int(name[len('grcov_lcov_output'):-len('.info')]) for name in z.namelist()]) + 1

This is needed for Linux but not for Windows. On Linux we run force-cargo-test-run more than once (once for geckodriver and once for the rest).

Comment 7

[(not currently active) Ted Mielczarek]

Comment on attachment 9012934 [details] [diff] [review]
Patch

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

I think you should switch to using buildconfig instead of mozinfo here, and I would feel better if you had a more straightforward way of specifying the path to grcov, but other than that this looks OK.

::: testing/runrusttests.py
@@ +3,5 @@
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +from __future__ import with_statement

You don't actually need this, we require Python 2.7.

@@ +32,5 @@
> +
> +def main():
> +    update_mozinfo()
> +
> +    if mozinfo.info['ccov']:

You could avoid the mozinfo dance by just using the build system substs directly:
```
import buildconfig

buildconfig.substs.get['MOZ_CODE_COVERAGE']
```

@@ +40,5 @@
> +        elif mozinfo.os == 'win':
> +            prefix = 'z:/build/build/src/'
> +            # Add 1 as on Windows the path where the compiler tries to write the
> +            # gcda files has an additional 'obj-firefox' component.
> +            strip_count = prefix.count('/') + 1

You ought to be able to handle this without the platform conditionals by just using `buildconfig.topsrcdir`.

@@ +54,5 @@
> +
> +    if mozinfo.info['ccov']:
> +        build_obj = MozbuildObject.from_environment()
> +
> +        grcov_path = os.path.join(os.environ['MOZ_FETCHES_DIR'], 'grcov')

Feels like it would be nicer to set the path to grcov directly in the environment or something like that. This will definitely break if someone uses `--enable-coverage` in a local build.

@@ +64,5 @@
> +            grcov_path,
> +            '-t', 'lcov',
> +            '-p', prefix,
> +            '--ignore-dir', 'gcc*',
> +            '--ignore-dir', 'vs2017_*',

These are paths to system headers from the toolchains? If so it might be nicer to derive these paths from somewhere vs. hardcoding these and hoping they don't change.

@@ +69,5 @@
> +            build_obj.topobjdir, gcov_dir
> +        ]
> +
> +        if mozinfo.os == 'win' or mozinfo.os == 'mac':
> +            grcov_command += ['--llvm']

What does this control? Do we want this enabled whenever we're using clang? If so we ought to be checking `substs['CC_TYPE'] in ('clang', 'clang-cl')` here.

@@ +77,5 @@
> +        shutil.rmtree(gcov_dir)
> +
> +        grcov_zip_path = os.path.join(build_obj.topobjdir, 'code-coverage-grcov.zip')
> +        with zipfile.ZipFile(grcov_zip_path, 'a', zipfile.ZIP_DEFLATED) as z:
> +            index = max([0] + [int(name[len('grcov_lcov_output'):-len('.info')]) for name in z.namelist()]) + 1

What do we do for code coverage info for the rest of the build? I wonder if it'd be better to just write the files to disk and then zip them all up after the build.

Comment 8

[Marco Castelluccio [:marco]]

(In reply to Ted Mielczarek [:ted] [:ted.mielczarek] from comment #7)
> @@ +40,5 @@
> > +        elif mozinfo.os == 'win':
> > +            prefix = 'z:/build/build/src/'
> > +            # Add 1 as on Windows the path where the compiler tries to write the
> > +            # gcda files has an additional 'obj-firefox' component.
> > +            strip_count = prefix.count('/') + 1
> 
> You ought to be able to handle this without the platform conditionals by
> just using `buildconfig.topsrcdir`.

The problem here is that, while prefix is `buildconfig.topsrcdir` on all OSes, on Linux and Mac the strip count is `prefix.count('/')`, on Windows it is `prefix.count('/') + 1`.

> 
> @@ +54,5 @@
> > +
> > +    if mozinfo.info['ccov']:
> > +        build_obj = MozbuildObject.from_environment()
> > +
> > +        grcov_path = os.path.join(os.environ['MOZ_FETCHES_DIR'], 'grcov')
> 
> Feels like it would be nicer to set the path to grcov directly in the
> environment or something like that. This will definitely break if someone
> uses `--enable-coverage` in a local build.

I could check if GRCOV_PATH is in env, and fallback to this when it isn't.

> 
> @@ +64,5 @@
> > +            grcov_path,
> > +            '-t', 'lcov',
> > +            '-p', prefix,
> > +            '--ignore-dir', 'gcc*',
> > +            '--ignore-dir', 'vs2017_*',
> 
> These are paths to system headers from the toolchains? If so it might be
> nicer to derive these paths from somewhere vs. hardcoding these and hoping
> they don't change.

I think I might have found an alternative solution to achieve the same result here, by ignoring all absolute paths (since the paths to mozilla-central files will be relative).

> 
> @@ +69,5 @@
> > +            build_obj.topobjdir, gcov_dir
> > +        ]
> > +
> > +        if mozinfo.os == 'win' or mozinfo.os == 'mac':
> > +            grcov_command += ['--llvm']
> 
> What does this control? Do we want this enabled whenever we're using clang?
> If so we ought to be checking `substs['CC_TYPE'] in ('clang', 'clang-cl')`
> here.

This is just an optimization that tells grcov that all coverage files where generated by LLVM (we're sure about that on Win and Mac), it's a very minor optimization so I'll just remove it.

> 
> @@ +77,5 @@
> > +        shutil.rmtree(gcov_dir)
> > +
> > +        grcov_zip_path = os.path.join(build_obj.topobjdir, 'code-coverage-grcov.zip')
> > +        with zipfile.ZipFile(grcov_zip_path, 'a', zipfile.ZIP_DEFLATED) as z:
> > +            index = max([0] + [int(name[len('grcov_lcov_output'):-len('.info')]) for name in z.namelist()]) + 1
> 
> What do we do for code coverage info for the rest of the build? I wonder if
> it'd be better to just write the files to disk and then zip them all up
> after the build.

For now, we only have these coverage files as we're only collecting them for Rust tests. We might start collecting them for other things we run during the build, so this might be worth it (maybe in a follow-up).

Comment 9

[Marco Castelluccio [:marco]]

Attachment: Patch

(In reply to Marco Castelluccio [:marco] from comment #8)
> (In reply to Ted Mielczarek [:ted] [:ted.mielczarek] from comment #7)
> > @@ +40,5 @@
> > > +        elif mozinfo.os == 'win':
> > > +            prefix = 'z:/build/build/src/'
> > > +            # Add 1 as on Windows the path where the compiler tries to write the
> > > +            # gcda files has an additional 'obj-firefox' component.
> > > +            strip_count = prefix.count('/') + 1
> > 
> > You ought to be able to handle this without the platform conditionals by
> > just using `buildconfig.topsrcdir`.
> 
> The problem here is that, while prefix is `buildconfig.topsrcdir` on all
> OSes, on Linux and Mac the strip count is `prefix.count('/')`, on Windows it
> is `prefix.count('/') + 1`.

We actually won't need all this as we're on a build machine for Rust tests.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/da6477e4baae
Run Rust tests in code coverage builds. r=ted
https://hg.mozilla.org/integration/mozilla-inbound/rev/9a1506ffe7ff
Download grcov in Linux and Windows build tasks. r=ted

Comment 11

[Marco Castelluccio [:marco]]

Attachment: Part 3: Parse coverage artifacts for Rust tests

I've landed the first two parts of the patch, which are self-contained and were approved by ted.

Comment 12

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/da6477e4baae
https://hg.mozilla.org/mozilla-central/rev/9a1506ffe7ff

Comment 13

[Marco Castelluccio [:marco]]

Forgot to set leave-open, there's still a patch pending.

Comment 14

[Marco Castelluccio [:marco]]

Ted, could you review or redirect the review?

Comment 15

[(not currently active) Ted Mielczarek]

Comment on attachment 9020378 [details] [diff] [review]
Part 3: Parse coverage artifacts for Rust tests

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

::: testing/parse_rust_tests_ccov.py
@@ +19,5 @@
> +        grcov_path = os.environ['GRCOV_PATH']
> +    else:
> +        grcov_path = os.path.join(os.environ['MOZ_FETCHES_DIR'], 'grcov')
> +        if buildconfig.substs['OS_TARGET'] == 'WINNT':
> +            grcov_path = os.path.join('z:\\build', grcov_path)

It would be nice if this was a little less hacky. Presumably it's hard to set absolute paths on the Windows builders? Can we just put the full 'z:\build' path in the task environment?

@@ +36,5 @@
> +        ignore_dir_abs = os.environ['VSWINPATH']
> +    else:
> +        ignore_dir_abs = os.path.dirname(os.path.dirname(buildconfig.substs['CC']))
> +
> +    # The glob passed to grcov must be relative to the source directory.

Why is this a requirement? What would break if we moved our toolchain packages out of topsrcdir? (which is something we've discussed in the past)

@@ +43,5 @@
> +    grcov_command += ['--ignore-dir', os.path.relpath(ignore_dir_abs, buildconfig.topsrcdir) + '*']
> +
> +    grcov_output = subprocess.check_output(grcov_command)
> +
> +    grcov_zip_path = os.path.join(buildconfig.topobjdir, 'code-coverage-grcov.zip')

If you don't have a specific need to use .zip files here you might consider using .tar.gz instead:
https://dxr.mozilla.org/mozilla-central/rev/d6ac0dc85306b753c1d1d5f3084f73f0d1286b86/python/mozbuild/mozbuild/action/test_archive.py#757

@@ +45,5 @@
> +    grcov_output = subprocess.check_output(grcov_command)
> +
> +    grcov_zip_path = os.path.join(buildconfig.topobjdir, 'code-coverage-grcov.zip')
> +    with zipfile.ZipFile(grcov_zip_path, 'a', zipfile.ZIP_DEFLATED) as z:
> +        index = max([0] + [int(name[len('grcov_lcov_output'):-len('.info')]) for name in z.namelist()]) + 1

This seems potentially error-prone if we run more than one set of tests in parallel (I don't know if that actually happens or not, though). Could we instead run this command at the end of the entire build?

Comment 16

[Marco Castelluccio [:marco]]

(In reply to Ted Mielczarek [:ted] [:ted.mielczarek] from comment #15)
> ::: testing/parse_rust_tests_ccov.py
> @@ +19,5 @@
> > +        grcov_path = os.environ['GRCOV_PATH']
> > +    else:
> > +        grcov_path = os.path.join(os.environ['MOZ_FETCHES_DIR'], 'grcov')
> > +        if buildconfig.substs['OS_TARGET'] == 'WINNT':
> > +            grcov_path = os.path.join('z:\\build', grcov_path)
> 
> It would be nice if this was a little less hacky. Presumably it's hard to
> set absolute paths on the Windows builders? Can we just put the full
> 'z:\build' path in the task environment?

MOZ_FETCHES_DIR is a relative directory on Windows. Perhaps we don't need to add z:\\build here if we run this script in the same working directory as when we download fetches.

> 
> @@ +36,5 @@
> > +        ignore_dir_abs = os.environ['VSWINPATH']
> > +    else:
> > +        ignore_dir_abs = os.path.dirname(os.path.dirname(buildconfig.substs['CC']))
> > +
> > +    # The glob passed to grcov must be relative to the source directory.
> 
> Why is this a requirement? What would break if we moved our toolchain
> packages out of topsrcdir? (which is something we've discussed in the past)

It isn't strictly a requirement. grcov assumes that any source file in the source directory belongs to the project, so if the toolchains are in the source directory they are considered as source files of the project. If we moved the toolchains out of topsrcdir we would also be able to remove all of this filtering code, but I've added the assertion so that I can verify, if that happens, that everything still works as expected.

> 
> @@ +43,5 @@
> > +    grcov_command += ['--ignore-dir', os.path.relpath(ignore_dir_abs, buildconfig.topsrcdir) + '*']
> > +
> > +    grcov_output = subprocess.check_output(grcov_command)
> > +
> > +    grcov_zip_path = os.path.join(buildconfig.topobjdir, 'code-coverage-grcov.zip')
> 
> If you don't have a specific need to use .zip files here you might consider
> using .tar.gz instead:
> https://dxr.mozilla.org/mozilla-central/rev/
> d6ac0dc85306b753c1d1d5f3084f73f0d1286b86/python/mozbuild/mozbuild/action/
> test_archive.py#757

For now we need to use zip, but we can switch to tar.gz at some point. I've already filed a bug about this, but I don't remember the bug #.

> 
> @@ +45,5 @@
> > +    grcov_output = subprocess.check_output(grcov_command)
> > +
> > +    grcov_zip_path = os.path.join(buildconfig.topobjdir, 'code-coverage-grcov.zip')
> > +    with zipfile.ZipFile(grcov_zip_path, 'a', zipfile.ZIP_DEFLATED) as z:
> > +        index = max([0] + [int(name[len('grcov_lcov_output'):-len('.info')]) for name in z.namelist()]) + 1
> 
> This seems potentially error-prone if we run more than one set of tests in
> parallel (I don't know if that actually happens or not, though). Could we
> instead run this command at the end of the entire build?

Agreed.

Comment 17

[Marco Castelluccio [:marco]]

(In reply to Ted Mielczarek [:ted] [:ted.mielczarek] from comment #15)
> @@ +45,5 @@
> > +    grcov_output = subprocess.check_output(grcov_command)
> > +
> > +    grcov_zip_path = os.path.join(buildconfig.topobjdir, 'code-coverage-grcov.zip')
> > +    with zipfile.ZipFile(grcov_zip_path, 'a', zipfile.ZIP_DEFLATED) as z:
> > +        index = max([0] + [int(name[len('grcov_lcov_output'):-len('.info')]) for name in z.namelist()]) + 1
> 
> This seems potentially error-prone if we run more than one set of tests in
> parallel (I don't know if that actually happens or not, though). Could we
> instead run this command at the end of the entire build?

I tried adding an additional @PostScriptRun in https://searchfox.org/mozilla-central/rev/f2028b4c38bff2a50ed6aa1763f6dc5ee62b0cc4/testing/mozharness/mozharness/mozilla/building/buildbase.py, which executes the new Python script I added in testing/, but I haven't had much luck.

On Linux grcov fails with:
> /builds/worker/fetches/grcov: /usr/lib/x86_64-linux-gnu/libstdc++.so.6: version `GLIBCXX_3.4.20' not found (required by /builds/worker/fetches/grcov)
> /builds/worker/fetches/grcov: /usr/lib/x86_64-linux-gnu/libstdc++.so.6: version `GLIBCXX_3.4.19' not found (required by /builds/worker/fetches/grcov)

(not sure why it fails like this if the Python script is run from buildbase.py, but not if I run it directly from config/rules.mk)

On Windows, the buildconfig data seems to be wrong (e.g. `buildconfig.substs.get('MOZ_CODE_COVERAGE')` is None).

What is another nice way to run a script at the end of the build? The other place I had found other than buildbase.py was the end of https://searchfox.org/mozilla-central/rev/f2028b4c38bff2a50ed6aa1763f6dc5ee62b0cc4/python/mozbuild/mozbuild/controller/building.py#962, not sure if that's OK.

Comment 18

[Marco Castelluccio [:marco]]

(In reply to Marco Castelluccio [:marco] from comment #17)
> (In reply to Ted Mielczarek [:ted] [:ted.mielczarek] from comment #15)
> > @@ +45,5 @@
> > > +    grcov_output = subprocess.check_output(grcov_command)
> > > +
> > > +    grcov_zip_path = os.path.join(buildconfig.topobjdir, 'code-coverage-grcov.zip')
> > > +    with zipfile.ZipFile(grcov_zip_path, 'a', zipfile.ZIP_DEFLATED) as z:
> > > +        index = max([0] + [int(name[len('grcov_lcov_output'):-len('.info')]) for name in z.namelist()]) + 1
> > 
> > This seems potentially error-prone if we run more than one set of tests in
> > parallel (I don't know if that actually happens or not, though). Could we
> > instead run this command at the end of the entire build?
> 
> I tried adding an additional @PostScriptRun in
> https://searchfox.org/mozilla-central/rev/
> f2028b4c38bff2a50ed6aa1763f6dc5ee62b0cc4/testing/mozharness/mozharness/
> mozilla/building/buildbase.py, which executes the new Python script I added
> in testing/, but I haven't had much luck.
> 
> On Linux grcov fails with:
> > /builds/worker/fetches/grcov: /usr/lib/x86_64-linux-gnu/libstdc++.so.6: version `GLIBCXX_3.4.20' not found (required by /builds/worker/fetches/grcov)
> > /builds/worker/fetches/grcov: /usr/lib/x86_64-linux-gnu/libstdc++.so.6: version `GLIBCXX_3.4.19' not found (required by /builds/worker/fetches/grcov)
> 
> (not sure why it fails like this if the Python script is run from
> buildbase.py, but not if I run it directly from config/rules.mk)
> 
> On Windows, the buildconfig data seems to be wrong (e.g.
> `buildconfig.substs.get('MOZ_CODE_COVERAGE')` is None).

I fixed the Linux issue by setting "os.environ['LD_LIBRARY_PATH'] = '%s/lib64/' % gcc_dir", there's an error in grcov now that I need to figure out.

Here's an example failure on Windows: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f2c003f99746fb10553f4f579288ea95a7f7ec5b.

Comment 19

[Marco Castelluccio [:marco]]

Attachment: Part 3: Parse coverage artifacts at the end of builds

I've managed to fix things on Windows, by running the command with the env from query_build_env().

I've stopped using 'CC' to get the path to gcc as it doesn't work when sccache is used. I think it's easier to just hardcode 'gcc' and add assertions and comments to explain what to do in the rare case it should fail.

Comment 20

[(not currently active) Ted Mielczarek]

Comment on attachment 9030427 [details] [diff] [review]
Part 3: Parse coverage artifacts at the end of builds

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

Thanks for sticking through all of my nitpicking, I think the final result here is much better!

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8882b6afe79f
Parse coverage artifacts at the end of builds. r=ted

Comment 22

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/8882b6afe79f