Closed Bug 1473067 Opened 7 years ago Closed 6 years ago

Run Rust tests in coverage builds

Categories

(Testing :: Code Coverage, enhancement)

enhancement
Not set
normal

Tracking

(firefox65 wontfix, firefox66 fixed)

RESOLVED FIXED
mozilla66
Tracking Status
firefox65 --- wontfix
firefox66 --- fixed

People

(Reporter: marco, Assigned: marco)

References

Details

Attachments

(1 file, 5 obsolete files)

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?
Attached patch Patch (obsolete) — Splinter Review
Assignee: nobody → mcastelluccio
Status: NEW → ASSIGNED
Attachment #8997098 - Flags: feedback?(ted)
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 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.
Attachment #8997098 - Flags: feedback?(ted) → feedback+
Depends on: 1492159
Attached patch Patch (obsolete) — Splinter Review
Attachment #8997098 - Attachment is obsolete: true
Attachment #9012361 - Flags: review?(ted)
Attached patch Patch (obsolete) — Splinter Review
(Updated after some recent changes around the toolchain/fetch tasks)
Attachment #9012361 - Attachment is obsolete: true
Attachment #9012361 - Flags: review?(ted)
Attachment #9012934 - Flags: review?(ted)
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 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.
Attachment #9012934 - Flags: review?(ted)
(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).
Attached patch Patch (obsolete) — Splinter Review
(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.
Attachment #9012934 - Attachment is obsolete: true
Attachment #9018604 - Flags: review?(ted)
Pushed by mcastelluccio@mozilla.com: 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
I've landed the first two parts of the patch, which are self-contained and were approved by ted.
Attachment #9018604 - Attachment is obsolete: true
Attachment #9018604 - Flags: review?(ted)
Attachment #9020378 - Flags: review?(ted)
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
Forgot to set leave-open, there's still a patch pending.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Ted, could you review or redirect the review?
Flags: needinfo?(ted)
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?
Attachment #9020378 - Flags: review?(ted)
(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.
Flags: needinfo?(ted)
(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.
(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.
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.
Attachment #9020378 - Attachment is obsolete: true
Attachment #9030427 - Flags: review?(ted)
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!
Attachment #9030427 - Flags: review?(ted) → review+
Pushed by mcastelluccio@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/8882b6afe79f Parse coverage artifacts at the end of builds. r=ted
Status: REOPENED → RESOLVED
Closed: 7 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: mozilla65 → mozilla66
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: