Closed Bug 1349640 Opened 7 years ago Closed 7 years ago

Figure out what to do with header files in build directory that are symbolic links to files in the source directory

Categories

(Testing :: Code Coverage, enhancement)

Version 3
enhancement
Not set
normal

Tracking

(firefox55 fixed)

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: marco, Assigned: chmanchester)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

The gcda/gcno files contain paths to the symbolic links instead of the files in the source tree.
This means that the web page that will show code coverage information will not be able to get those files from the mercurial repository.
I assume the build process creates these links?  Do we know where in the build they are created?  Can we make a good guesses, encode those guesses into data, and use that data during ETL?
We have a script here in this bug [1] which rewrites the symbolic source file names to local paths in JSON coverage artifacts. Maybe this work could help with the gcno/gcda files also.

[1]: https://bugzilla.mozilla.org/show_bug.cgi?id=1255179
(In reply to Greg Mierzwinski [:gmierz] from comment #2)
> We have a script here in this bug [1] which rewrites the symbolic source
> file names to local paths in JSON coverage artifacts. Maybe this work could
> help with the gcno/gcda files also.
> 
> [1]: https://bugzilla.mozilla.org/show_bug.cgi?id=1255179

This is solving a slightly different issue... if I understand this bug, the information we want to access is contained in install manifests. We can probably emulate the code in symbolstore.py:  http://searchfox.org/mozilla-central/rev/9af9b1c7946ebef6056d2ee4c368d496395cf1c8/toolkit/crashreporter/tools/symbolstore.py#319-329 :
(In reply to Chris Manchester (:chmanchester) from comment #3)
> This is solving a slightly different issue... 

Ah I see, thanks!

In the UCOSP meeting this evening, Kyle asked whether this could be done on his end with the ETL. Would this be possible or should this be done before then? Also, where are these install manifests located, are they artifacts or files in the source tree?
Yes. If there is an artifact that can be uploaded (or is already uploaded) that can be used to perform the symbolic link mapping, then it is best that it is done in the ETL layer: We want to keep the Code coverage collection as simple as possible (let the ETL be responsible for information aggregation and collation), while leaving the database and UI to be simple and direct.  

The caveat is: I ask you be explicit about how to interpret those artifacts; I imagine a Vidyo call, with a few links, is the fastest way to teach how to interpret the data.
Depends on: 1350310
Assignee: nobody → cmanchester
Here's a quick patch on try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=771d96edbcc96ef63f759d5b70f93a5287ba241c

It uploads a json file (ending in "linked-files-map.json") mapping from objdir relative paths under dist/include to srcdir relative paths: 

{
...
"dist/include/mozilla/BasicEvents.h": "widget/BasicEvents.h",
"dist/include/mozilla/dom/ChromeUtils.h": "dom/base/ChromeUtils.h",
...
}

etc.


Kyle, Marco, is this going to work?
The format looks ok. Please zip the file so it is smaller.

The mapping appears more complex than I had imagined. It looks like multiple directories get dumped into one for the compilation. How are filename collisions avoided?! Well, I guess it's not my problem.

Thank you!
(In reply to Kyle Lahnakoski [:ekyle] from comment #7)
> The format looks ok. Please zip the file so it is smaller.
> 
> The mapping appears more complex than I had imagined. It looks like multiple
> directories get dumped into one for the compilation. How are filename
> collisions avoided?! Well, I guess it's not my problem.
> 
> Thank you!

It may be simpler for this code to simply include the json file in the main gcno archive, does that work for you?
Flags: needinfo?(klahnakoski)
Yes, good idea.  The mapping can go into the zipped gcno file.
Flags: needinfo?(klahnakoski)
Comment on attachment 8852090 [details]
Bug 1349640 - Allow the code coverage archiver to accept multiple patterns.

https://reviewboard.mozilla.org/r/124332/#review127242

Looks good, but see also the issue in the other review on whether this is necessary.

::: toolkit/mozapps/installer/packager.mk:80
(Diff revision 1)
>  ifdef MOZ_CODE_COVERAGE
>  	# Package code coverage gcno tree
>  	@echo 'Packaging code coverage data...'
>  	$(RM) $(CODE_COVERAGE_ARCHIVE_BASENAME).zip
>  	$(PYTHON) -mmozbuild.codecoverage.packager \
> -		--output-file='$(DIST)/$(PKG_PATH)$(CODE_COVERAGE_ARCHIVE_BASENAME).zip'
> +        --output-file='$(DIST)/$(PKG_PATH)$(CODE_COVERAGE_ARCHIVE_BASENAME).zip' \

nit: This becomes really difficult to read when viewed with a tabsize of 8. Each of the commands are then indented to the same width as the --output-file and --input-pattern arguments, which make those arguments look like they are separate commands. Can we keep the original indentation of --output-file and match it for --input-pattern?
Attachment #8852090 - Flags: review?(mshal) → review+
Comment on attachment 8852091 [details]
Bug 1349640 - Upload a mapping for headers in dist/include for the benefit of code coverage builds.

https://reviewboard.mozilla.org/r/124334/#review127244

I'm fine with landing this if the nits are fixed, though it may be simpler to combine the two files. Up to you if you want to pursue that or not.

::: python/mozbuild/mozbuild/action/summarize_install_manifest.py:5
(Diff revision 1)
> +# 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/.
> +
> +# Given a Python script and arguments describing the output file, and

Looks like this was inadvertently copied from file_generate.py. Either update it for this script or just remove it.

::: python/mozbuild/mozbuild/action/summarize_install_manifest.py:23
(Diff revision 1)
> +    UnreadableInstallManifest,
> +)
> +import mozpack.path as mozpath
> +
> +
> +def main(argv):

Why not just put this as a separate function in codecoverage/packager.py? Then I don't think you'd even need the first commit and can just add it to the jar along with **/*.gcno

::: toolkit/mozapps/installer/packager.mk:79
(Diff revision 1)
>  endif # MOZ_ARTIFACT_BUILD_SYMBOLS
>  ifdef MOZ_CODE_COVERAGE
> +	@echo 'Writing map of files linked to dist/include...'
> +	$(RM) $(DEPTH)/linked-files-map.json
> +	$(call py_action,summarize_install_manifest, \
> +        --manifest=$(DEPTH)/_build_manifests/install/dist_include \

Similar spacing issue here with tabs shown as 8 spaces.
Attachment #8852091 - Flags: review?(mshal) → review+
Comment on attachment 8852091 [details]
Bug 1349640 - Upload a mapping for headers in dist/include for the benefit of code coverage builds.

https://reviewboard.mozilla.org/r/124334/#review127244

> Why not just put this as a separate function in codecoverage/packager.py? Then I don't think you'd even need the first commit and can just add it to the jar along with **/*.gcno

Yep, that sounds reasonable. Making this more general isn't really serving a purpose right now.
Attachment #8852090 - Attachment is obsolete: true
Attachment #8852091 - Flags: review+ → review?(mshal)
Comment on attachment 8852091 [details]
Bug 1349640 - Upload a mapping for headers in dist/include for the benefit of code coverage builds.

https://reviewboard.mozilla.org/r/124334/#review127742

Code-wise it looks good to me, though I noticed that the linked-files-map.json file changed slightly. Previously it included things like dist/include/Crypto.h -> dom/base/Crypto.h (like #c6), but now it is browser/installer/dist/include/Crypto.h -> dom/base/Crypto.h. Was this intentional?

::: python/mozbuild/mozbuild/codecoverage/packager.py:56
(Diff revision 2)
> +                                         '_build_manifests',
> +                                         'install',
> +                                         'dist_include')
> +    linked_files = describe_install_manifest(dist_include_manifest,
> +                                             'dist/include')
> +    mapping_file = GeneratedFile(json.dumps(linked_files))

Would it help at all to sort the linked_files so the output is consistent?
Attachment #8852091 - Flags: review?(mshal) → review+
Comment on attachment 8852091 [details]
Bug 1349640 - Upload a mapping for headers in dist/include for the benefit of code coverage builds.

https://reviewboard.mozilla.org/r/124334/#review127742

Nope! Looks like I something somewhere shouldn't be a relative path. Thanks for noticing that.

> Would it help at all to sort the linked_files so the output is consistent?

Sure
Pushed by cmanchester@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/167ed7d3545b
Upload a mapping for headers in dist/include for the benefit of code coverage builds. r=mshal
https://hg.mozilla.org/mozilla-central/rev/167ed7d3545b
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Blocks: 1305262
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: