Closed Bug 1259832 Opened 8 years ago Closed 7 years ago

Save generated files so Socorro can link to them

Categories

(Firefox Build System :: General, defect)

defect
Not set
normal

Tracking

(firefox57 fixed)

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: blassey, Assigned: ted)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

Attachments

(4 files)

A lot of the crashes I've looked at recently have been occurring in generated code in the object directory. For example, this crash report [https://crash-stats.mozilla.com/report/index/e56984e3-1431-4858-bf44-612e52160312] unhelpfully points to https://hg.mozilla.org/releases/mozilla-beta/annotate/fb3494d06dfb/obj-firefox/ipc/ipdl/PHttpChannelChild.cpp#l1080. It would be great if we could save these generated files somewhere and link to that instead.
The "obvious" suggestion would be for dxr to build all the revisions we use for release-type builds, and soccoro to point there.

Erik, how possible would that be on dxr side?
Flags: needinfo?(erik)
I've wanted to do this for a while, I've just never come up with a good plan. We'd mostly just need to get the list of generated files from the build system and upload them somewhere, and confer some of that information to the symbolstore script so that it can make the FILE lines have enough information to generate the links.
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #2)
> I've wanted to do this for a while, I've just never come up with a good
> plan. We'd mostly just need to get the list of generated files from the
> build system and upload them somewhere, and confer some of that information
> to the symbolstore script so that it can make the FILE lines have enough
> information to generate the links.

in symbolstore script it should just be anything with obj-firefox in the path
Mike, we do index generated files, e.g. https://dxr.mozilla.org/mozilla-central/source/obj-x86_64-unknown-linux-gnu/ipc/ipdl/PHttpChannelChild.cpp. And we just added Aurora, Beta, Release, and ESR45. Does that suffice?
Flags: needinfo?(erik)
(In reply to Erik Rose [:erik][:erikrose] from comment #4)
> Mike, we do index generated files, e.g.
> https://dxr.mozilla.org/mozilla-central/source/obj-x86_64-unknown-linux-gnu/
> ipc/ipdl/PHttpChannelChild.cpp. And we just added Aurora, Beta, Release, and
> ESR45. Does that suffice?

That's a good start. Ideally, we should have the exact revisions shipped to users indexed. Can we ensure that?

Now, an interesting additional question is: do we have build-time generated code that varies per platform?
Flags: needinfo?(erik)
We can set up a DXR tree for each release tag. It should cost us around 14GB of elasticsearch space per tag, but the CPU impact should be negligible, since the code doesn't change. I expect the tags could eventually rotate out after a year or so--sane? How does all that sound to you?

We do index only x86_64 Linux right now, as (1) our analysis shims are tied to clang and (2) we haven't figured out how to merge indices across multiple builds yet. If that's going to be a showstopper, please say so.
Flags: needinfo?(erik)
(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #3)
> in symbolstore script it should just be anything with obj-firefox in the path

I don't think that's *quite* sufficient, unfortunately.

And yeah, I'm not sure that we don't have generated code that varies by platform.
it seems to me that dxr might be the wrong direction then. I'm thinking the right thing to do is to markup the produced source and save it as a build artifact in ftp.
Note not having this is particularly annoying for Stylo because we have lots of code generated for property parsing, computation, animation, etc. Many of our crash reports end in functions inside generated files.
Depends on: 1384568
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #10)
> Note not having this is particularly annoying for Stylo because we have lots
> of code generated for property parsing, computation, animation, etc. Many of
> our crash reports end in functions inside generated files.

Can you show me an example of these generated files? I'm working on a patch in bug 1384568 and I want to make sure it captures this use case.
dmajor pointed out properties.rs, which is generated from this stylo build script:
https://dxr.mozilla.org/mozilla-central/source/servo/components/style/build.rs

using this Python script:
https://dxr.mozilla.org/mozilla-central/source/servo/components/style/properties/build.py

Unfortunately the Gecko build system doesn't know anything about what cargo build scripts do internally. :-(
I think Gecko build system can just try to keep all files in objdir/toolkit/library/<target-triplet>/{debug,release}/build/*/out. That is the place build scripts put generated files by default.
Depends on: 1384889
Depends on: 1385887
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #14)
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=a5944a5aed51eb35771fec8874c6f4ae32c696ea

This try push implements one of the two parts I'm intending to use to fix this. It produces a `generated-sources.tar.gz` artifact from builds. The second part will involve adding a new task to fetch that artifact and upload the individual files within to a new S3 bucket.
OK, I think I have this working, I just need to try actually executing the task.

I took the parameters.yml from a m-c build that included nightlies:
https://queue.taskcluster.net/v1/task/au1EPpAFR6-Fl1ftKeLWSw/runs/0/artifacts/public/parameters.yml

and ran `./mach taskgraph tasks --json` with it, and then applied my patch and ran it again. This is the diff between the two outputs:
https://gist.github.com/anonymous/10a95d1ee5e8370fb97f963ae234b8d7

I also ran the upload_generated_sources.py script locally with an S3 bucket I set up for testing and it was able to upload the files successfully. Once I get builds for the try push in the previous comment I'm going to manually create a task using the task-creator and the task definitions from running `mach taskgraph tasks` locally, hand-editing the task definition to point at the artifacts from my try push, so I can verify that the upload works when fetching AWS creds from the secrets service.
Looking at my try push I realize that:
+            "task-reference": "public/build/target.generated-sources.tar.gz@<build>"

is incorrect, the first patch produces target.generated-files.tar.gz.
My `upload_generated_sources.py` script didn't actually work--I forgot to make it activate a virtualenv since in this context it will be running without an existing objdir. I fixed that (and tested by running it locally without an existing objdir) and did another try push.
...so I also was missing the `--bucket` in the task commandline, and I had the TC secrets URL wrong in the script. It's a wonder I ever write code that works.
Assignee: nobody → ted
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #16)
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=c72e11f1e5fc9390d448544150123b9518120fc0

One thing I forgot to mention--this push broke artifact builds because they're trying to run the package-generated-sources build step. I need to make them skip that.
Depends on: 1388820
Blocks: 1389217
Blocks: 1389225
OK! I think this is all good to go. I triggered a try push on that review request just to make sure I didn't break anything with my recent changes, and I'll manually trigger an upload task when I get a build out of it.

I filed a pair of followups:
* bug 1389217 to implement the actual links in Socorro, which should be trivial
* bug 1389225 to make source server indexing work for these files, so Windows debuggers can fetch them
The builds on that try push failed because I shadowed a variable in the code I added in symbolstore.py which only broke one thing: the printing of symbol file names. That broke the functional test.
Comment on attachment 8895974 [details]
bug 1259832 - add a post-build task to upload generated source files.

https://reviewboard.mozilla.org/r/167244/#review172774

I made some changes in bug 1382729 to, among other things, use $MOZ_SCM_LEVEL here.  Given the backouts I saw on that bug, I think it's wise to adopt those changes here, too, before landing.

I can set up some buckets and secrets for that purpose.
Attachment #8895974 - Flags: review?(dustin) → review+
buckets / users / secrets created
Comment on attachment 8895972 [details]
bug 1259832 - move artifact build automation step overrides to a common mozconfig.

https://reviewboard.mozilla.org/r/167240/#review172844
Attachment #8895972 - Flags: review?(cmanchester) → review+
Comment on attachment 8895975 [details]
bug 1259832 - replace generated source file names in symbol files.

https://reviewboard.mozilla.org/r/167246/#review172846
Attachment #8895975 - Flags: review?(cmanchester) → review+
(In reply to Dustin J. Mitchell [:dustin] from comment #35)
> Comment on attachment 8895974 [details]
> bug 1259832 - add a post-build task to upload generated source files.
> 
> https://reviewboard.mozilla.org/r/167244/#review172774
> 
> I made some changes in bug 1382729 to, among other things, use
> $MOZ_SCM_LEVEL here.  Given the backouts I saw on that bug, I think it's
> wise to adopt those changes here, too, before landing.
> 
> I can set up some buckets and secrets for that purpose.

I agree.

(In reply to Dustin J. Mitchell [:dustin] from comment #36)
> buckets / users / secrets created

Thanks!
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #34)
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=0eeae57188be1f6f13c609be31a899e4bf7b3994

This was mostly green, but the Windows builds failed my new test in unit-symbolstore.py because I used `os.path.join` and wound up with unexpected backslashes. I'll change that to use `mozpath.join` instead.
New buckets are gecko-generated-sources-l1 and gecko-generated-sources-l2.  The l3 bucket hasn't changed - no suffix.
Comment on attachment 8895973 [details]
bug 1259832 - package generated sources and upload them along with other build artifacts.

https://reviewboard.mozilla.org/r/167242/#review174104

::: python/mozbuild/mozpack/generated_sources.py:1
(Diff revision 1)
> +# This Source Code Form is subject to the terms of the Mozilla Public

This file should not be in `mozpack`, since `mozpack` is somewhat generic.

::: python/mozbuild/mozpack/generated_sources.py:7
(Diff revision 1)
> +# 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 json
> +import os
> +import os.path

Nit: no need to `import os.path` when there is an `import os`.

::: python/mozbuild/mozpack/generated_sources.py:21
(Diff revision 1)
> +    in this objdir, where `file` is either an absolute path to the file or
> +    a `mozpack.File` instance.
> +    '''
> +    # First, get the list of generated sources produced by the build backend.
> +    f = os.path.join(buildconfig.topobjdir, 'generated-sources.json')
> +    data = json.load(open(f, 'rb'))

Nit: always use context manager form of `open()` to ensure file descriptors are closed immediately.

::: python/mozbuild/mozpack/generated_sources.py:27
(Diff revision 1)
> +    for p, f in finder.find('**/*.h'):
> +        yield os.path.join(base, p), f

I'm really not a fan of doing a filesystem walk here. That I/O can be expensive. Can we not derive the list of files from moz.build metadata? I'd rather we generate the list of relevant files at backend generation time and then consume that list from this script.

That being said, since the filesystem walking seems to be limited to specific sub-directories, I'm willing to turn a blind eye to I/O perf concerns.
Attachment #8895973 - Flags: review?(gps) → review-
Comment on attachment 8895973 [details]
bug 1259832 - package generated sources and upload them along with other build artifacts.

https://reviewboard.mozilla.org/r/167242/#review174104

> This file should not be in `mozpack`, since `mozpack` is somewhat generic.

OK, I originally only had the `get_generated_files` function here, which seemed fairly generic, but I stuffed a few other things in here in the interim so I can put this elsewhere.

> I'm really not a fan of doing a filesystem walk here. That I/O can be expensive. Can we not derive the list of files from moz.build metadata? I'd rather we generate the list of relevant files at backend generation time and then consume that list from this script.
> 
> That being said, since the filesystem walking seems to be limited to specific sub-directories, I'm willing to turn a blind eye to I/O perf concerns.

Per bug 1384568 comment 1, the moz.build emitter doesn't have any visibility into these specific cases. I tried to limit it to the couple of very specific directories where we had generated source files that aren't captured in generated-sources.json.

The ipdlheaders one shouldn't be too bad, I think we effectively want to capture every file in that directory. The Rust sources one isn't great, but the directory tree isn't very deep from the point where we're walking, and stylo builds have some important generated sources in there that we really do want to capture.

I'd love to have a more declarative solution here, but unfortunately we just don't have it right now.
Comment on attachment 8895974 [details]
bug 1259832 - add a post-build task to upload generated source files.

dustin: if you could give this a quick second look I'd appreciate it. I changed it to use the bucket for the SCM level of the task. I just pushed to try and requested Nightly builds, so we'll see if that actually works.
Attachment #8895974 - Flags: review+ → review?(dustin)
Comment on attachment 8895973 [details]
bug 1259832 - package generated sources and upload them along with other build artifacts.

https://reviewboard.mozilla.org/r/167242/#review174246
Attachment #8895973 - Flags: review?(gps) → review+
dustin: did a project/releng/gecko/build/level-1/gecko-generated-sources-upload secret actually get created?
I don't see it in the secrets manager, and my Ugs task on that last try push 404ed on it:
https://treeherder.mozilla.org/logviewer.html#?job_id=123391630&repo=try&lineNumber=81


I see project/releng/gecko/build/level-{2,3}/gecko-generated-sources-upload in the secrets manager.
Flags: needinfo?(dustin)
Sorry, I think I didn't set the expiration on the secret correctly and it expired.  I re-created it.
Flags: needinfo?(dustin)
Comment on attachment 8895974 [details]
bug 1259832 - add a post-build task to upload generated source files.

https://reviewboard.mozilla.org/r/167244/#review174514
Attachment #8895974 - Flags: review?(dustin) → review+
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #57)
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=dece32af44a89517fc24f633652f89121b1b72c8

I did in fact get green Ugs tasks on this try push! (The Win32 build timed out, but it also took 49 minutes(!) to clone the repo, so that's not my fault.) However, looking at the log from the Win64 Ugs task I noticed:
[task 2017-08-16T18:55:05.434551Z] INFO - Thread-1 - Uploading "6b2e6cfe5e37b0e2446a577fea1df1922d4fb7345e2716f5c994de35f3d5562b6792a16aedcb14d33659f9c68b5713f335e03d7865e0c519cd421e604cf6a9f2/toolkit/library\x86_64-pc-windows-msvc\release\build\style-b085cfb8bb9e43c9/out/properties.rs" (423127 bytes)

and sure enough, downloading the target.generated-files.tar.gz and looking at it:
$ tar tzf target.generated-files.tar.gz  | grep properties\.rs
toolkit/library\\x86_64-pc-windows-msvc\\release\\build\\style-b085cfb8bb9e43c9/out/gecko_properties.rs
toolkit/library\\x86_64-pc-windows-msvc\\release\\build\\style-b085cfb8bb9e43c9/out/properties.rs

I messed up the path separators in the archive generation. :-( Fixing that shouldn't be terrible, I will fix that and do a Windows-only try push to sanity check.
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #59)
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=78344e2435cc2379d4b202b71851d1112236066b

That looks better:
[task 2017-08-16T23:31:22.397418Z] INFO - Thread-1 - Uploading "7ccab0fcaada36876ba2d7bf686c8b77907ab791fea2e3e9c87347d1e828ec147b44a24b42b8af3abca072f03b635194b73a24f6b11f3c3a552e5be4782514b4/toolkit/library/i686-pc-windows-msvc/release/build/style-853b9d8a7b7bd19c/out/gecko/bindings.rs" (12783 bytes)
https://hg.mozilla.org/integration/mozilla-inbound/rev/a02695cbf5762eb0eb7087239319807eb447ca1e
bug 1259832 - move artifact build automation step overrides to a common mozconfig. r=chmanchester

https://hg.mozilla.org/integration/mozilla-inbound/rev/14d18d7cf454c4c3d0f6d49d1d01660e06e4be4b
bug 1259832 - package generated sources and upload them along with other build artifacts. r=gps

https://hg.mozilla.org/integration/mozilla-inbound/rev/73bf88110b3821d62a3d393e85b56896a12f2930
bug 1259832 - add a post-build task to upload generated source files. r=dustin

https://hg.mozilla.org/integration/mozilla-inbound/rev/7781a37a4db0378b06ef3ad377e18be37e63ad83
bug 1259832 - replace generated source file names in symbol files. r=chmanchester
Backed these out for apparently scheduling nightlies on inbound for every push and platform

https://hg.mozilla.org/integration/mozilla-inbound/rev/82d8999c065b
Flags: needinfo?(ted)
Something in this or the other bug you pushed busted android checkstyle,findbugs,lint,"test" jobs like https://treeherder.mozilla.org/logviewer.html#?job_id=123725305&repo=mozilla-inbound
(In reply to Wes Kocher (:KWierso) from comment #63)
> Something in this or the other bug you pushed busted android
> checkstyle,findbugs,lint,"test" jobs like
> https://treeherder.mozilla.org/logviewer.html#?job_id=123725305&repo=mozilla-
> inbound

Thanks. Looks like I need to skip the new automation build step here as well:
https://dxr.mozilla.org/mozilla-central/source/mobile/android/config/mozconfigs/android-api-15-frontend/nightly?q=path%3Amozconfigs%2Fandroid-api-15-frontend%2Fnightly&redirect_type=single#1

Not sure why I didn't notice this on try.
Flags: needinfo?(ted)
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #64)
> Not sure why I didn't notice this on try.

I'm going to go with "because pushing to try with -b do -p all does not schedule those jobs". :-(
Comment on attachment 8895974 [details]
bug 1259832 - add a post-build task to upload generated source files.

https://reviewboard.mozilla.org/r/167244/#review175082

::: taskcluster/ci/upload-generated-sources/kind.yml:20
(Diff revisions 2 - 3)
>  only-for-attributes:
>    - nightly
>  
>  job-template:
>    description: Upload generated source files from build
> +  attributes:

This was the fix to prevent these tasks from being scheduled on inbound, and thus scheduling nightly builds for their own use.

::: taskcluster/taskgraph/transforms/upload_generated_sources.py:32
(Diff revisions 2 - 3)
>          # Label the job to match the build task it's uploading from.
>          job['label'] = dep_task.label.replace("build-", "upload-generated-sources-")
> -        # Copy over some bits of treeherder metdata from the build task.
> +        # Copy over some bits of metdata from the build task.
>          dep_th = dep_task.task['extra']['treeherder']
> +        job.setdefault('attributes', {})
> +        job['attributes']['build_platform'] = dep_task.attributes.get('build_platform')

This fix was necessary to get these jobs scheduled from the actual nightly build cron job scheduler. In prior testing I had forced the tasks to run on try (which worked), and verified that they showed up in the output of `mach taskgraph tasks`, which lists *all* tasks, but neither of those were sufficient to verify that they would get scheduled along with actual nightly builds in automation. I needed to use `mach taskgraph target` with a parameters.yml from a nightly cron decision task to verify this.
Comment on attachment 8895973 [details]
bug 1259832 - package generated sources and upload them along with other build artifacts.

https://reviewboard.mozilla.org/r/167242/#review175086

I had to add `MOZ_AUTOMATION_PACKAGE_GENERATED_SOURCES=0` to some mobile mozconfigs. For mobile artifact builds I simply included the mozconfig.artifact.automation I added in the earlier patch (those changes could have gone into the earlier patch, but here they are). For the couple of other one-off build types I simply included it inline.
Comment on attachment 8895973 [details]
bug 1259832 - package generated sources and upload them along with other build artifacts.

Pretty minimal additional changes here to just skip the new automation build step in some Android builds that I missed because they don't run on try.
Attachment #8895973 - Flags: review+ → review?(gps)
Comment on attachment 8895974 [details]
bug 1259832 - add a post-build task to upload generated source files.

dustin: I made two changes to this patch from the previous patch, I noted them both in mozreview.
Attachment #8895974 - Flags: review+ → review?(dustin)
Comment on attachment 8895973 [details]
bug 1259832 - package generated sources and upload them along with other build artifacts.

https://reviewboard.mozilla.org/r/167242/#review175356
Attachment #8895973 - Flags: review?(gps) → review+
Comment on attachment 8895974 [details]
bug 1259832 - add a post-build task to upload generated source files.

https://reviewboard.mozilla.org/r/167244/#review175502
Attachment #8895974 - Flags: review?(dustin) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/7fffe26efe40f211687e6cefe8b166e35835ef3a
bug 1259832 - move artifact build automation step overrides to a common mozconfig. r=chmanchester

https://hg.mozilla.org/integration/mozilla-inbound/rev/1f1dd3ab877d9733604aee3a2bcb66eccf70eb9e
bug 1259832 - package generated sources and upload them along with other build artifacts. r=gps

https://hg.mozilla.org/integration/mozilla-inbound/rev/4a6df9405fb5b6a89373e71e4118fc03712c9458
bug 1259832 - add a post-build task to upload generated source files. r=dustin

https://hg.mozilla.org/integration/mozilla-inbound/rev/3cd6728702e32f02a1201c4d83aa60676fc902d7
bug 1259832 - replace generated source file names in symbol files. r=chmanchester
This doesn't look like it's doing anything dumb on inbound, which is good. We'll have to wait for it to get merged to central and for nightlies to be built with it included to see if it actually does what I want. Given the success of my recent try pushes (most recently in comment 65) I have high confidence that that part will work.
This is mostly-working, in that there were Ugs tasks scheduled for the nightly builds yesterday, like:
https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&filter-searchStr=nightly&selectedJob=124377496

However, having these frames show up in crash stacks breaks Socorro's report/index page (oops), which peterbe is fixing in bug 1389217.

Also, after looking at the logs/symbols from those nightlies, I don't think this is working properly for Windows builds right now. My best guess is that it's an issue with backslashes. I'll file a followup to fix that, and I've already done a local build to try it out on Windows.
Blocks: 1392312
Blocks: 1393240
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/a0f8d0262f8b
Keep build files in sync (Port bug 1259832: package generated sources and upload them along with other build artifacts). rs=bustage-fix
Product: Core → Firefox Build System
Blocks: 1717976
You need to log in before you can comment on or make changes to this bug.