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

Put debug symbols in releases #5370

Merged
merged 7 commits into from
Oct 30, 2024

Conversation

expipiplus1
Copy link
Collaborator

Closes #5267

@expipiplus1 expipiplus1 added the pr: non-breaking PRs without breaking changes label Oct 22, 2024
jkwak-work
jkwak-work previously approved these changes Oct 22, 2024
Copy link
Collaborator

@jkwak-work jkwak-work left a 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.

.github/workflows/ci.yml Show resolved Hide resolved
@csyonghe
Copy link
Collaborator

Have you verified this works on windows? From the build log it is still outputting to the Release folder.

@expipiplus1
Copy link
Collaborator Author

Have you verified this works on windows? From the build log it is still outputting to the Release folder.

Where are you observing this, in CI it's compiling into RelWithDebInfo
image

I changed things to make RelWithDebInfo the default config btw

@expipiplus1 expipiplus1 force-pushed the push-vosskotvovrv branch 4 times, most recently from cdcee82 to f515561 Compare October 23, 2024 05:02
@csyonghe
Copy link
Collaborator

There are a lot of warnings on windows builds:

cl : Command line warning D9025 : overriding '/MD' with '/MT'

@expipiplus1
Copy link
Collaborator Author

yeah, I'm looking into that now

@expipiplus1 expipiplus1 force-pushed the push-vosskotvovrv branch 2 times, most recently from 9f5c469 to f3c693e Compare October 24, 2024 03:48
@expipiplus1
Copy link
Collaborator Author

ok, looks like it's fixed

clean.sh Outdated Show resolved Hide resolved
@expipiplus1
Copy link
Collaborator Author

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

@csyonghe
Copy link
Collaborator

Question: when this lands, what is our prebuilt binary package directory structure? Are things under bin/RelWithDebInfo/ instead of bin/Release. I really don't like RelWithDebInfo.

@expipiplus1
Copy link
Collaborator Author

image
This has been the layout for a while now, no Release or RelWithDebInfo either way

In the build tree it'll look like build/RelWithDebInfo/bin/* yes. It's very possible with CMake to change that to build/bin for example (or build/Foo/bin) if that's something you want?

Copy link
Collaborator

@csyonghe csyonghe left a 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?

@csyonghe
Copy link
Collaborator

The package layout is good, no need for changes then.

@expipiplus1
Copy link
Collaborator Author

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)

@csyonghe
Copy link
Collaborator

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.

@csyonghe csyonghe merged commit 44dc5ea into shader-slang:master Oct 30, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr: non-breaking PRs without breaking changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Slang release builds on windows has no debug info.
3 participants