Closed
Bug 1473423
Opened 6 years ago
Closed 6 years ago
cppunittest symbols are excluded from crashreporter-symbols-full.zip
Categories
(Firefox Build System :: General, enhancement)
Firefox Build System
General
Tracking
(firefox63 fixed)
RESOLVED
FIXED
mozilla63
Tracking | Status | |
---|---|---|
firefox63 | --- | fixed |
People
(Reporter: bugzilla, Assigned: bugzilla)
References
Details
Attachments
(1 file)
3.46 KB,
patch
|
ted
:
review+
|
Details | Diff | Splinter Review |
I've got a crashing cppunittest that only fails when built using our CI; I can't reproduce this with a local build. Unfortunately (on try at least) it looks like we aren't saving the PDBs for cppunittests anywhere, so this is a lot harder for me to debug than it otherwise would be. We *do* save the breakpad symbols for the cppunittests into crashreporter-symbols.zip. Can we either fix this omission, or else add the cppunittest PDBs to a separate artifact? I don't really care too much as long as I can access them somehow.
Comment 1•6 years ago
|
||
I think we did this because they bloated the symbol archive and they didn't seem useful since we don't ship those to users. Now that we can upload symbols from try builds things are a bit different. I'd be fine with always including them on try builds since try build artifacts aren't stored for very long anyway. Currently it's just these --exclude bits that control that behavior: https://dxr.mozilla.org/mozilla-central/rev/6c0fa9a675c91390ca27664ffb626c56e8afea4d/Makefile.in#268 It's probably a PITA to put a conditional there, but that just invokes this Python script: https://dxr.mozilla.org/mozilla-central/rev/6c0fa9a675c91390ca27664ffb626c56e8afea4d/python/mozbuild/mozbuild/action/symbols_archive.py I would propose that we change the arguments to that script to get rid of --exclude/--include/--compress and simply have a `--full-archive` option or something like that, so that the `symbolsfullarchive` target could pass that to include everything, and the `symbolsarchive` target would not pass it and would only include .sym files. We could then have the script internally check whether it's a try build and if so skip the filtering.
Assignee | ||
Comment 2•6 years ago
|
||
(In reply to Ted Mielczarek [:ted] [:ted.mielczarek] from comment #1) > We could then have the script internally > check whether it's a try build and if so skip the filtering. I have a patch written that does everything except for this: What's the best way for me to check that it's a try build? MH_BRANCH from the environment? Something else?
Flags: needinfo?(ted)
Comment 3•6 years ago
|
||
Awesome! Yeah, that's probably the most sensible way. You'll want to use `os.environ.get` so that it has graceful fallback for non-automation builds, like: https://dxr.mozilla.org/mozilla-central/rev/085cdfb90903d4985f0de1dc7786522d9fb45596/mobile/android/mach_commands.py#419
Flags: needinfo?(ted)
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → aklotz
Status: NEW → ASSIGNED
Assignee | ||
Comment 4•6 years ago
|
||
Attachment #8993462 -
Flags: review?(core-build-config-reviews)
Comment 5•6 years ago
|
||
Comment on attachment 8993462 [details] [diff] [review] Allow test symbols to be included in symbol archives on try Review of attachment 8993462 [details] [diff] [review]: ----------------------------------------------------------------- ::: python/mozbuild/mozbuild/action/symbols_archive.py @@ +43,5 @@ > + if args.full_archive: > + # We allow symbols for tests to be included when building on try > + if os.environ.get('MH_BRANCH', 'unknown') != 'try': > + excludes = ['*test*', '*Test*'] > + compress = ['**/*.sym'] Considering that compress defaults to this and you've now removed the option, maybe just remove the compress argument to `make_archive` and hardcode it to this value? https://dxr.mozilla.org/mozilla-central/rev/085cdfb90903d4985f0de1dc7786522d9fb45596/python/mozbuild/mozbuild/action/symbols_archive.py#19
Attachment #8993462 -
Flags: review?(core-build-config-reviews) → review+
Comment 6•6 years ago
|
||
I forgot to say: this looks really nice!
Assignee | ||
Comment 7•6 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/3268a54a7fab9e9565038d589229e9f2bf1cdff9 Bug 1473423: Allow test symbols to be included in full symbol archive in try builds; r=ted
Comment 8•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/3268a54a7fab
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox63:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in
before you can comment on or make changes to this bug.
Description
•