-
Notifications
You must be signed in to change notification settings - Fork 246
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
Put debug symbols in releases #5370
Conversation
0b1d39b
to
e52a5d5
Compare
e52a5d5
to
262f43d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me.
Have you verified this works on windows? From the build log it is still outputting to the Release folder. |
cdcee82
to
f515561
Compare
There are a lot of warnings on windows builds:
|
yeah, I'm looking into that now |
9f5c469
to
f3c693e
Compare
ok, looks like it's fixed |
3f70af5
to
fc4ca2f
Compare
will need to change the 'release' rules here to 'releaseWithDebugInfo' once this is merged. https://github.com/shader-slang/slang/settings/branch_protection_rules/1102224 |
Question: when this lands, what is our prebuilt binary package directory structure? Are things under |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On Linux, are the debug symbols embedded into the so? Can we separate the debug symbols into their own files like on windows?
The package layout is good, no need for changes then. |
It's definitely possible to do separate debug info on linux (but no on MacOS I think?), but it's not very common in my experience. In fact, I think at the moment we're embedding the debug info on Windows as well (sccache doesn't support separate debug info iirc) |
IIRC the binaries with debug info embedded is very big, something about 50MB? Not sure if it actually is a good idea to include it for all of our users, but it is probably still the better choice? We could provide a separate withDebInfo package for Linux and windows x64 only. |
Closes #5267