Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ci: generate debug symbols on Linux #18676

Merged
merged 7 commits into from Nov 21, 2019

Conversation

CapOM
Copy link
Contributor

@CapOM CapOM commented Jun 6, 2019

Description of Change

For mac and win there is currently electron-v4.2.3-darwin-x64-dsym.zip and electron-v4.2.3-win32-x64-pdb.zip of about 500MB. There is no such debug package for linux. This PR just fixes that.

Electron uses breakpad to generate minidumps but there are cases where it does not work properly on Linux so better to use usual coredumps and proper debug symbols on Linux.

#13315

Checklist

Release Notes

Notes: Generate debug symbols on Linux

@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Jun 6, 2019
@CapOM CapOM changed the title Generate debug symbols linux Generate debug symbols on Linux Jun 6, 2019
@CapOM
Copy link
Contributor Author

CapOM commented Jun 6, 2019

Yes it has generated the new debug.zip file: https://circleci.com/gh/electron/electron/220774?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-checks-link

Failures on Mac and Win seem unrelated to this PR

@CapOM CapOM changed the title Generate debug symbols on Linux ci: generate debug symbols on Linux Jun 6, 2019
@CapOM
Copy link
Contributor Author

CapOM commented Jun 6, 2019

Hi @alexeykuzmin , Hi @MarshallOfSound , who will be able to review this change ? Thx a lot.

@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Jun 6, 2019
.circleci/config.yml Outdated Show resolved Hide resolved
appveyor.yml Outdated Show resolved Hide resolved
script/strip-binaries.py Outdated Show resolved Hide resolved
script/zip-symbols.py Show resolved Hide resolved
script/strip-binaries.py Outdated Show resolved Hide resolved
@alexeykuzmin
Copy link
Contributor

@CapOM Thank you for the PR! It's definitely something that can be really useful.
I left a few comments in the code though.

@nornagon nornagon mentioned this pull request Jun 10, 2019
3 tasks
Copy link
Contributor

@jkleinsc jkleinsc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of including this in the strip binaries step/script this should be done in its own step/script that gets executed before strip binaries.

@CapOM CapOM force-pushed the generate_debug_symbols_linux branch 2 times, most recently from 40b79a4 to 347ee92 Compare June 26, 2019 20:13
@CapOM
Copy link
Contributor Author

CapOM commented Jun 26, 2019

Hi I addressed the remarks, added a separate script, please take a look, thx!

Copy link
Member

@MarshallOfSound MarshallOfSound left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you want these to be uploaded in the release you'll have to update both the upload.py script and the validate-release script (to check that the assets exist)

@CapOM CapOM force-pushed the generate_debug_symbols_linux branch from 347ee92 to f9efe5c Compare June 28, 2019 16:18
@CapOM CapOM requested a review from a team as a code owner June 28, 2019 16:18
@CapOM
Copy link
Contributor Author

CapOM commented Jun 28, 2019

Hi @MarshallOfSound , thx for your comment. I pushed what you suggested. Please take a look to script/release/uploaders/upload.py and script/release/release.js.

@CapOM
Copy link
Contributor Author

CapOM commented Jul 2, 2019

Hi, gentle ping ?

Copy link
Member

@MarshallOfSound MarshallOfSound left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change looks reasonable I'd like some thought from @nornagon as they're currently refactoring our symbol generation 👍

Copy link
Member

@nornagon nornagon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems handy, just a few style comments.

FYI, #19042 refactors some symbol dumping stuff for breakpad symbols, but I don't think it'll conflict with this.

script/copy-debug-symbols.py Outdated Show resolved Hide resolved
script/copy-debug-symbols.py Outdated Show resolved Hide resolved
script/copy-debug-symbols.py Outdated Show resolved Hide resolved
.circleci/config.yml Outdated Show resolved Hide resolved
script/lib/config.py Outdated Show resolved Hide resolved
script/copy-debug-symbols.py Outdated Show resolved Hide resolved
@CapOM CapOM force-pushed the generate_debug_symbols_linux branch from f9efe5c to c0756d6 Compare July 3, 2019 23:10
@CapOM
Copy link
Contributor Author

CapOM commented Jul 3, 2019

Hi @nornagon , thx for the review. I addressed all of your remarks. Please take a look, thx!

Copy link
Member

@nornagon nornagon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Real close! Thanks for working on this :)

ping @jkleinsc, can we get a release dry run on this PR?

script/release/uploaders/upload.py Show resolved Hide resolved
.circleci/config.yml Outdated Show resolved Hide resolved
@CapOM CapOM force-pushed the generate_debug_symbols_linux branch 3 times, most recently from 9240529 to 8b99934 Compare July 12, 2019 15:40
@jkleinsc
Copy link
Contributor

Release builds for this PR here:
CircleCI release build request for linux-arm-publish successful. Check https://circleci.com/gh/electron/electron/261930 for status.
CircleCI release build request for mas-publish successful. Check https://circleci.com/gh/electron/electron/261934 for status.
CircleCI release build request for linux-arm64-publish successful. Check https://circleci.com/gh/electron/electron/261931 for status.
CircleCI release build request for linux-ia32-publish successful. Check https://circleci.com/gh/electron/electron/261935 for status.
CircleCI release build request for linux-x64-publish successful. Check https://circleci.com/gh/electron/electron/261932 for status.
CircleCI release build request for osx-publish successful. Check https://circleci.com/gh/electron/electron/261933 for status.
AppVeyor release build request for electron-x64 successful. Check build status at https://ci.appveyor.com/project/electron-bot/electron-x64-release/build/1.0.113
AppVeyor release build request for electron-ia32 successful. Check build status at https://ci.appveyor.com/project/electron-bot/electron-ia32-release/build/1.0.86

cc @nornagon @CapOM

@CapOM CapOM force-pushed the generate_debug_symbols_linux branch from 8b99934 to 276fe5e Compare July 15, 2019 17:26
@trop
Copy link
Contributor

trop bot commented Nov 25, 2019

I was unable to backport this PR to "8-x-y" cleanly;
you will need to perform this backport manually.

@trop
Copy link
Contributor

trop bot commented Nov 25, 2019

@nornagon has manually backported this PR to "8-x-y", please check out #21278

@trop
Copy link
Contributor

trop bot commented Nov 25, 2019

@nornagon has manually backported this PR to "7-1-x", please check out #21279

@trop
Copy link
Contributor

trop bot commented Nov 25, 2019

@nornagon has manually backported this PR to "6-1-x", please check out #21280

jkleinsc pushed a commit that referenced this pull request Dec 3, 2019
* ci: generate debug symbols on Linux (#18676)

* kick ci
@sofianguy sofianguy added this to Fixed in 8.0.0-beta.4 in 8.2.x Dec 9, 2019
@sofianguy sofianguy added this to Fixed in 6.1.6 in 6.1.x Feb 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
6.1.x
Fixed in 6.1.6
8.2.x
Fixed in 8.0.0-beta.4
Development

Successfully merging this pull request may close these issues.

None yet

7 participants