Upload zipped archive of all .gcno files to build directory if code coverage is enabled

RESOLVED FIXED in mozilla34

Status

defect
RESOLVED FIXED
5 years ago
Last year

People

(Reporter: ahal, Assigned: ahal)

Tracking

unspecified
mozilla34
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

To get code coverage builds going in tbpl, we need to upload all .gcno files (while preserving directory structure) to ftp.m.o. My understanding is the upload step happens in the build system itself.

From #releng:

<ahal> so with the new mozharness build scripts, does the uploading of the installer/tests to ftp.m.o happen in mozharness? Or is that done by buildbot
<jlund> ahal: so 'mach build' does almost all the post build things :)
<mshal> well, we haven't enabled the 'mach build'-does-all-the-things yet in m-c :)
<mshal> if it's a new step entirely we'll want to add it to moz-automation.mk
<ahal> mshal: I'm trying to get code coverage builds stood up.. after the build we'll need to walk the directory tree, collect all .gcov files, zip them up and upload them to the build dir
<mshal> ahal: ahh. Well you might want to look at the top-level Makefile.in at a target like buildsymbols
<mshal> ahal: and basically create a new target that does the steps you want. Then we can work that into the automation
<mshal> ahal: you can see the output from buildsymbols (SYMBOL_ARCHIVE_BASENAME.zip) gets added to UPLOAD_FILES in m-c/toolkit/mozapps/installer/packager.mk. You'd probably want the same with your gcov zip file if you want it in the same place
<mshal> (same place on ftp, I mean)
A cheap way to do this would be to tack something on to the end of the build that tars up all the files, and puts the resulting tar file in $(DIST), and then you should just be able to list the file in MOZ_UPLOAD_FILES:
http://hg.mozilla.org/mozilla-central/annotate/a9b43778f0c2/toolkit/mozapps/installer/packager.mk#l876
Are you going to *run* stuff during the build? I don't think you would, so *gcno files wouldn't be a byproduct of the build, but of test runs. So this is quite a different setup to get those back.
(In reply to Mike Hommey [:glandium] from comment #2)
> Are you going to *run* stuff during the build? I don't think you would, so
> *gcno files wouldn't be a byproduct of the build, but of test runs. So this
> is quite a different setup to get those back.

.gcno files are a byproduct of the build.
.gcda files are a byproduct of the tests.

Both are needed.
Assignee: nobody → ahalberstadt
Status: NEW → ASSIGNED
This patch adds a make target for finding, copying and compressing all the .gcno files in the objdir.

A few notes:
1) This assumes a MOZ_CODE_COVERAGE variable gets defined, so depends on bug 1054275.
2) I dumped a python script in tools/code-coverage, but maybe this should be in mozbuild somewhere?
3)  I'm still trying to figure out how to get the build system to invoke this automatically after the build is complete (I think it's something to do with build/moz-automation.mk).
Attachment #8473829 - Flags: feedback?(gps)
Comment on attachment 8473829 [details] [diff] [review]
Add make target for building a .gcno tree

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

::: Makefile.in
@@ +257,5 @@
> +
> +	@echo packing code-coverage gcno tree
> +	$(NSINSTALL) -D $(DIST)/$(PKG_PATH)
> +	cd $(DIST)/code-coverage-gcno && \
> +		  zip -r9D '../$(PKG_PATH)$(CODE_COVERAGE_ARCHIVE_BASENAME).zip' .

We decided a while ago that we're trying to do away with all this "shell in makefile" foo and replace it with Python, preferably Python living in importable modules (as opposed to scripts because reusability).

You can use the zipfile module (https://docs.python.org/2/library/zipfile.html) to produce zip files directly from Python. Using it, you will avoid the I/O of staging .gcno files to a common directory. You can just write out a .zip file into its final location while reading .gcno files from wherever they reside.

This approach is also more testable. We like test coverage [of code coverage].

::: tools/code-coverage/build_gcno_tree.py
@@ +1,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/.
> +
> +import argparse

Let's stick the logic in this file and from Makefile.in in mozbuild.codecoverage (python/mozbuild/mozbuild/codecoverage).

@@ +11,5 @@
> +
> +def build_gcno_tree(root, outdir):
> +    finder = FileFinder(root)
> +    copier = FileCopier()
> +    for p, f in finder.find("**/*.gcno"):

Filesystem walking is a mostly legitimate solution. However, the build system already knows what binaries it creates as part of the build. Can we not produce a set of .gcno files during config.status and look for them directly? This has the following advantages:

1) Faster. Filesystem walking can be expensive.
2) Only includes files that are part of the build.
Attachment #8473829 - Flags: feedback?(gps)
Instead of using the zipfile module, you should use the Jarrer class in mozbuild.mozpack.copier, with optimize=False.
(In reply to Gregory Szorc [:gps] from comment #5)
> Filesystem walking is a mostly legitimate solution. However, the build
> system already knows what binaries it creates as part of the build. Can we
> not produce a set of .gcno files during config.status and look for them
> directly? This has the following advantages:

That implies that you absolutely know every source file [that you care about] that gets compiled from moz.build. Note that NSPR and NSS, among other libraries, are not in this category. Whether or not you care about not being able to measure coverage from NSPR, NSS, et al is a different question.
Thanks for the feedback! I was just copying the buildsymbols make target as it was doing something similar to what I needed.

(In reply to Gregory Szorc [:gps] from comment #5)
> ::: Makefile.in
> @@ +257,5 @@
> > +
> > +	@echo packing code-coverage gcno tree
> > +	$(NSINSTALL) -D $(DIST)/$(PKG_PATH)
> > +	cd $(DIST)/code-coverage-gcno && \
> > +		  zip -r9D '../$(PKG_PATH)$(CODE_COVERAGE_ARCHIVE_BASENAME).zip' .
> 
> We decided a while ago that we're trying to do away with all this "shell in
> makefile" foo and replace it with Python, preferably Python living in
> importable modules (as opposed to scripts because reusability).
> 
> Let's stick the logic in this file and from Makefile.in in
> mozbuild.codecoverage (python/mozbuild/mozbuild/codecoverage).

Sounds good. To get this running as a build step, would I invoke this from http://mxr.mozilla.org/mozilla-central/source/build/moz-automation.mk ? Or is there some place in python I can hook in from?

> Filesystem walking is a mostly legitimate solution. However, the build
> system already knows what binaries it creates as part of the build. Can we
> not produce a set of .gcno files during config.status and look for them
> directly? This has the following advantages:
> 
> 1) Faster. Filesystem walking can be expensive.
> 2) Only includes files that are part of the build.

Assuming the point :jcranmer raised isn't a concern, this makes sense to me. I have no idea how to do that though :p.
(In reply to Andrew Halberstadt [:ahal] from comment #8)
> > Let's stick the logic in this file and from Makefile.in in
> > mozbuild.codecoverage (python/mozbuild/mozbuild/codecoverage).
> 
> Sounds good. To get this running as a build step, would I invoke this from
> http://mxr.mozilla.org/mozilla-central/source/build/moz-automation.mk ? Or
> is there some place in python I can hook in from?

The moz-automation.mk file is something new that we're trying, but it currently is only used on cedar. You'll want to add it in as a new step in buildbot (see: http://mxr.mozilla.org/build/source/buildbotcustom/process/factory.py#1081 ). Though if you are able to add it to moz-automation.mk as well, that would be appreciated :)
Do we need to add this to buildbot? I would think that we could overload |make package| to create the .zip and add it to the uploaded files set. But I'm not an expert in this part of the build system...
(In reply to Gregory Szorc [:gps] from comment #10)
> Do we need to add this to buildbot? I would think that we could overload
> |make package| to create the .zip and add it to the uploaded files set. But
> I'm not an expert in this part of the build system...

Sure, you can probably do that instead. It would be simpler since you don't have to fiddle with buildbot/moz-automation, but just might be overloading the term "package". I don't have a strong preference.
There's precedent here, FWIW. We use stage-package to package the JS shell which gets uploaded as part of UPLOAD_FILES:
http://hg.mozilla.org/mozilla-central/annotate/cbbc380f1e1c/toolkit/mozapps/installer/packager.mk#l738
If I go the non-buildbot route, then this depends on bug 1054275 (which I've run into a wall with). That's fine by me, but I'll need some help from someone with more build experience than I.
Posted patch WIP upload_gcno_tree (obsolete) — Splinter Review
Here's a WIP that moves the logic into mozbuild/codecoverage. I'm hitting a UnicodeDecodeError in mozjar.py though:

Traceback (most recent call last):
  File "/home/ahal/tmp/test.py", line 2, in <module>
    package_gcno_tree()
  File "/home/ahal/hg/mozilla-central/python/mozbuild/mozbuild/codecoverage/packager.py", line 23, in package_gcno_tree
    jarrer.copy(name)
  File "/home/ahal/hg/mozilla-central/python/mozbuild/mozpack/copier.py", line 484, in copy
    jar.preload(self._preload)
  File "/home/ahal/hg/mozilla-central/python/mozbuild/mozpack/mozjar.py", line 486, in __exit__
    self.finish()
  File "/home/ahal/hg/mozilla-central/python/mozbuild/mozpack/mozjar.py", line 556, in finish
    self._data.write(headers[entry].serialize())
  File "/home/ahal/hg/mozilla-central/python/mozbuild/mozpack/mozjar.py", line 146, in serialize
    serialized += self[name]
UnicodeDecodeError: 'ascii' codec can't decode byte 0x9b in position 14: ordinal not in range(128)
Attachment #8473829 - Attachment is obsolete: true
Why is there a gcno file with a non-ascii name?
(In reply to Mike Hommey [:glandium] from comment #15)
> Why is there a gcno file with a non-ascii name?

I'm not really sure what is going on. It doesn't seem to be the name that is non-ascii, but the serialized variable here:
http://mxr.mozilla.org/mozilla-central/source/python/mozbuild/mozpack/mozjar.py#134

"PK!<��I�Ut��"
I discovered there is a bug in JarWriter when the FileRegistry contains unicode strings. I filed bug 1056859 for it. Casting build_obj.topobjdir to <str> fixed the problem for me.
Posted patch upload_gcno_tree (obsolete) — Splinter Review
This patch seems to work for me locally (though haven't verified that it gets uploaded yet). Pushed it to Cedar to test it out:
https://tbpl.mozilla.org/?tree=Cedar&rev=074f50a4533e
Attachment #8476239 - Attachment is obsolete: true
Posted patch upload_gcno_tree (obsolete) — Splinter Review
There was a small typo in the last patch preventing the .zip from being uploaded. This patch fixes it. I also passed in 'PKG_BASENAME' from the Makefile to the python script instead of re-building it in MozbuildObject.
Attachment #8477430 - Attachment is obsolete: true
Attachment #8478993 - Flags: review?(gps)
Comment on attachment 8478993 [details] [diff] [review]
upload_gcno_tree

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

Almost.

::: python/mozbuild/mozbuild/codecoverage/__init__.py
@@ +1,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 .packager import package_gcno_tree

I'm not a huge fan of this Python pattern of aliasing everything into the package. Please make this an empty file unless you can point me at best practices that say I'm wrong.

::: python/mozbuild/mozbuild/codecoverage/packager.py
@@ +9,5 @@
> +import sys
> +
> +from mozpack.files import FileFinder
> +from mozpack.copier import Jarrer
> +from buildconfig import topobjdir

buildconfig is only available after we run configure and/or config.status. We therefore try to avoid importing it at module level in modules.

@@ +13,5 @@
> +from buildconfig import topobjdir
> +
> +def package_gcno_tree(output_file):
> +    # XXX JarWriter doesn't support unicode strings, see bug 1056859
> +    finder = FileFinder(topobjdir.encode('utf-8'))

topobjdir should be a function argument. Whether it comes from CLI arguments or you load buildconfig from cli() is your call.

::: toolkit/mozapps/installer/packager.mk
@@ +745,5 @@
> +ifdef MOZ_CODE_COVERAGE
> +	# Package code coverage gcno tree
> +	@echo 'Packaging code coverage data...'
> +	$(RM) $(CODE_COVERAGE_ARCHIVE_BASENAME).zip
> +	$(PYTHON) $(MOZILLA_DIR)/python/mozbuild/mozbuild/codecoverage/packager.py \

$(PYTHON) -mmozbuild.codecoverage.packager ...
Attachment #8478993 - Flags: review?(gps) → feedback+
Fixes review comments.
Attachment #8478993 - Attachment is obsolete: true
Attachment #8479991 - Flags: review?(gps)
Comment on attachment 8479991 [details] [diff] [review]
upload_gcno_tree

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

Looks good! Thanks for persevering.

BTW, I'm not sure what you are using to produce patches, but there are no patch headers in this attachment. It would be nice to see commit message, etc.
Attachment #8479991 - Flags: review?(gps) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/688501a435ac

Sorry, I usually format my patches just before pushing, which is a bad habit because the uploaded patches don't have the message. I'll try to remember to format them pre-upload in the future.
https://hg.mozilla.org/mozilla-central/rev/688501a435ac
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
I don't know where to file a bug on this, so I'll mention it here instead.

I've noticed that the code-coverage builds on Cedar seem to be failing due to the installer name picking up the code-coverage.gcno.zip as the installer instead of the actual installer:
<https://treeherder.mozilla.org/ui/logviewer.html#?job_id=80333&repo=cedar>.
Hmm, I vaguely remember running into this and fixing it awhile back. For the record, I haven't been working on this due to other priorities popping up, but I plan to pick it back up again soon. Thanks for pointing it out!
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.