Closed
Bug 1349640
Opened 8 years ago
Closed 8 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)
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.
Comment 1•8 years ago
|
||
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?
Comment 2•8 years ago
|
||
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
Assignee | ||
Comment 3•8 years ago
|
||
(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 :
Comment 4•8 years ago
|
||
(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?
Comment 5•8 years ago
|
||
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.
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → cmanchester
Assignee | ||
Comment 6•8 years ago
|
||
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?
Comment 7•8 years ago
|
||
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!
Assignee | ||
Comment 8•8 years ago
|
||
(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)
Comment 9•8 years ago
|
||
Yes, good idea. The mapping can go into the zipped gcno file.
Flags: needinfo?(klahnakoski)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 12•8 years ago
|
||
mozreview-review |
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 13•8 years ago
|
||
mozreview-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+
Assignee | ||
Comment 14•8 years ago
|
||
mozreview-review-reply |
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.
Comment hidden (mozreview-request) |
Assignee | ||
Updated•8 years ago
|
Attachment #8852090 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8852091 -
Flags: review+ → review?(mshal)
Comment 16•8 years ago
|
||
mozreview-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
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+
Assignee | ||
Comment 17•8 years ago
|
||
mozreview-review-reply |
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
Comment hidden (mozreview-request) |
Comment 19•8 years ago
|
||
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
Comment 20•8 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in
before you can comment on or make changes to this bug.
Description
•