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

Create and install PDB debug symbols alongside binaries #1037

Merged
merged 2 commits into from Mar 3, 2016

Conversation

Projects
None yet
6 participants
@Bromeon
Member

Bromeon commented Dec 30, 2015

Concerns PDB (program database) files -- debug symbols generated by Visual Studio

This modification outputs the generated PDB files alongside the static/import libraries in the lib directory. In SFML projects, it resolves the linker warning 4099, which cannot be disabled otherwise.

warning LNK4099: PDB 'vc140.pdb' was not found with ...; linking object as if no debug info

The minimally required CMake version is raised to 3.1, because the target properties COMPILE_PDB_NAME and COMPILE_PDB_OUTPUT_DIRECTORY are not available in older versions.

This pull request is a cleaner alternative to #980, as it properly generates the PDB files and doesn't just pack debug information into the static/import libraries.

Btw, I labeled this as feature and not bugfix because the primary purpose is to provide debug symbols for SFML, the fixed warning is just a side effect... but we can also re-label it, I don't care ;)

@Bromeon Bromeon added this to the 2.4 milestone Dec 30, 2015

@MarioLiebisch

This comment has been minimized.

Show comment
Hide comment
@MarioLiebisch

MarioLiebisch Dec 30, 2015

Member

The minimally required CMake version is raised to 3.1, because the target properties COMPILE_PDB_NAME and COMPILE_PDB_OUTPUT_DIRECTORY are not available in older versions.

How about version checking? It's not like the warning breaks older versions of CMake. Or would CMake even accept unknown target properties? Never tried.

Member

MarioLiebisch commented Dec 30, 2015

The minimally required CMake version is raised to 3.1, because the target properties COMPILE_PDB_NAME and COMPILE_PDB_OUTPUT_DIRECTORY are not available in older versions.

How about version checking? It's not like the warning breaks older versions of CMake. Or would CMake even accept unknown target properties? Never tried.

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon Dec 30, 2015

Member

An explicit check against the CMake version with an error message would be the only reasonable approach. Letting CMake fail with some obscure error/warning message (if at all) will only make people end up with half-broken configurations and wonder "where are my PDB files"...

However, the CMake configuration code is already a giant patchwork of case differentiations now, I'm not convinced that we should add another layer to check CMake itself. If we do this for all features that can be somehow worked around in older versions, the complexity will explode.

Member

Bromeon commented Dec 30, 2015

An explicit check against the CMake version with an error message would be the only reasonable approach. Letting CMake fail with some obscure error/warning message (if at all) will only make people end up with half-broken configurations and wonder "where are my PDB files"...

However, the CMake configuration code is already a giant patchwork of case differentiations now, I'm not convinced that we should add another layer to check CMake itself. If we do this for all features that can be somehow worked around in older versions, the complexity will explode.

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Dec 30, 2015

Member

Is it just for people that compile SFML themselves, or do you intend to ship them in SFML releases too?

Member

LaurentGomila commented Dec 30, 2015

Is it just for people that compile SFML themselves, or do you intend to ship them in SFML releases too?

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon Dec 31, 2015

Member

If we ship them for the VS configuration, people using the debug version of SFML are able to step in and don't get annoyed by LNK4099 all the time.

So that would be nice, however the .pdb files increase the package size (+16MB raw, +3.3MB zip, +1.8MB 7z)... Since not everybody uses debug symbols, maybe we should leave it for those recompiling SFML.

Member

Bromeon commented Dec 31, 2015

If we ship them for the VS configuration, people using the debug version of SFML are able to step in and don't get annoyed by LNK4099 all the time.

So that would be nice, however the .pdb files increase the package size (+16MB raw, +3.3MB zip, +1.8MB 7z)... Since not everybody uses debug symbols, maybe we should leave it for those recompiling SFML.

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Dec 31, 2015

Member

If we ship them for the VS configuration, people using the debug version of SFML are able to step in

Only if they have the source code too, which is distributed separately.

Member

LaurentGomila commented Dec 31, 2015

If we ship them for the VS configuration, people using the debug version of SFML are able to step in

Only if they have the source code too, which is distributed separately.

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon Dec 31, 2015

Member

Good point. So their primary use in the package is to silence the linker warning (and apparently only for static linking)...

I wonder that there were so few complaints about those linker errors in the past -- I would assume there are a lot of VS users, aren't there? Or do people just not care about warnings? They annoyed me to death so that I really felt the urge to do something about it 😁

Member

Bromeon commented Dec 31, 2015

Good point. So their primary use in the package is to silence the linker warning (and apparently only for static linking)...

I wonder that there were so few complaints about those linker errors in the past -- I would assume there are a lot of VS users, aren't there? Or do people just not care about warnings? They annoyed me to death so that I really felt the urge to do something about it 😁

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Dec 31, 2015

Member

The minimally required CMake version is raised to 3.1

This means that compiling SFML on Ubuntu prior to 15.10 (Wily) would require a manual installation of cmake, wouldn't it?

Member

mantognini commented Dec 31, 2015

The minimally required CMake version is raised to 3.1

This means that compiling SFML on Ubuntu prior to 15.10 (Wily) would require a manual installation of cmake, wouldn't it?

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon Dec 31, 2015

Member

This means that compiling SFML on Ubuntu prior to 15.10 (Wily) would require a manual installation of cmake, wouldn't it?

Okay, that's not cool. It's not nice if everybody has to pay only for PDB files on Visual Studio.

Looking forward to the manual case differentiation then 😜

Member

Bromeon commented Dec 31, 2015

This means that compiling SFML on Ubuntu prior to 15.10 (Wily) would require a manual installation of cmake, wouldn't it?

Okay, that's not cool. It's not nice if everybody has to pay only for PDB files on Visual Studio.

Looking forward to the manual case differentiation then 😜

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Dec 31, 2015

Member

Also, it fails building on OS X because starting with cmake 3.0 the compiler detection has changed, apparently, so this line is no longer valid.

Member

mantognini commented Dec 31, 2015

Also, it fails building on OS X because starting with cmake 3.0 the compiler detection has changed, apparently, so this line is no longer valid.

@MarioLiebisch

This comment has been minimized.

Show comment
Hide comment
@MarioLiebisch

MarioLiebisch Dec 31, 2015

Member

How about forcing the newer CMake version only on Windows? Wouldn't that solve both problems? On Windows you'll need manual installation anyway.

Member

MarioLiebisch commented Dec 31, 2015

How about forcing the newer CMake version only on Windows? Wouldn't that solve both problems? On Windows you'll need manual installation anyway.

@zsbzsb

This comment has been minimized.

Show comment
Hide comment
@zsbzsb

zsbzsb Dec 31, 2015

Member

Why not just keep it simple? If the user wants PDB files give them an option to check that they want the files. Then if the user checks the option either print an error if CMake version < 3.1 or generate the PDB files.

Member

zsbzsb commented Dec 31, 2015

Why not just keep it simple? If the user wants PDB files give them an option to check that they want the files. Then if the user checks the option either print an error if CMake version < 3.1 or generate the PDB files.

@MarioLiebisch

This comment has been minimized.

Show comment
Hide comment
@MarioLiebisch

MarioLiebisch Dec 31, 2015

Member

IIRC the problem is the fact that with no name given, the files are generated with a default name, which might be overwritten when building multiple projects from CMake (at least that's the problem I ran into several times with my own projects).

Member

MarioLiebisch commented Dec 31, 2015

IIRC the problem is the fact that with no name given, the files are generated with a default name, which might be overwritten when building multiple projects from CMake (at least that's the problem I ran into several times with my own projects).

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Dec 31, 2015

Member

If this whole thing is just to silence warnings with static linking on Visual C++... then is it really worth it? Or couldn't we just provide them as an additional package on the download page?

Member

LaurentGomila commented Dec 31, 2015

If this whole thing is just to silence warnings with static linking on Visual C++... then is it really worth it? Or couldn't we just provide them as an additional package on the download page?

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon Dec 31, 2015

Member

If the user wants PDB files give them an option to check that they want the files. Then if the user checks the option either print an error if CMake version < 3.1 or generate the PDB files.

I would also say this is the way to go. I can add a CMake variable SFML_GENERATE_PDB which concerns only the Visual Studio generator. For everybody else, things stay the same.

If this whole thing is just to silence warnings with static linking on Visual C++... then is it really worth it? Or couldn't we just provide them as an additional package on the download page?

It allows debugging SFML and silences the warning. It's worth it in my opinion, because the Debug libraries are quite useless otherwise (they're only required because Release libraries on Windows aren't ABI-compatible). And even with an additional package, we leave everybody who recompiles SFML in the dark, because the PDB files must match the generated binaries.

Member

Bromeon commented Dec 31, 2015

If the user wants PDB files give them an option to check that they want the files. Then if the user checks the option either print an error if CMake version < 3.1 or generate the PDB files.

I would also say this is the way to go. I can add a CMake variable SFML_GENERATE_PDB which concerns only the Visual Studio generator. For everybody else, things stay the same.

If this whole thing is just to silence warnings with static linking on Visual C++... then is it really worth it? Or couldn't we just provide them as an additional package on the download page?

It allows debugging SFML and silences the warning. It's worth it in my opinion, because the Debug libraries are quite useless otherwise (they're only required because Release libraries on Windows aren't ABI-compatible). And even with an additional package, we leave everybody who recompiles SFML in the dark, because the PDB files must match the generated binaries.

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon Jan 1, 2016

Member

Reverted minimum required version to 2.8.3, added SFML_GENERATE_PDB variable for VS and implemented a dynamic version check.

Member

Bromeon commented Jan 1, 2016

Reverted minimum required version to 2.8.3, added SFML_GENERATE_PDB variable for VS and implemented a dynamic version check.

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Jan 3, 2016

Member

So how about adding an option for including the debug symbols in the libs then as well?

Member

eXpl0it3r commented Jan 3, 2016

So how about adding an option for including the debug symbols in the libs then as well?

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon Jan 3, 2016

Member

Do you think it's still a requested feature, once the PDBs are properly generated? I wouldn't add it unless users actually need that option.

Member

Bromeon commented Jan 3, 2016

Do you think it's still a requested feature, once the PDBs are properly generated? I wouldn't add it unless users actually need that option.

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Jan 8, 2016

Member

No more complains from me. =P

Member

mantognini commented Jan 8, 2016

No more complains from me. =P

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Mar 1, 2016

Member

Do you think it's still a requested feature, once the PDBs are properly generated?

I personally like self-contained things (less files to move around and deal with), so adding it as advanced option wouldn't seem that bad to me. Guess we could discuss this somewhere else though.

This PR has been added to my merge list, meaning it will be merged soon, unless someone raises any concerns.

Member

eXpl0it3r commented Mar 1, 2016

Do you think it's still a requested feature, once the PDBs are properly generated?

I personally like self-contained things (less files to move around and deal with), so adding it as advanced option wouldn't seem that bad to me. Guess we could discuss this somewhere else though.

This PR has been added to my merge list, meaning it will be merged soon, unless someone raises any concerns.

@eXpl0it3r eXpl0it3r self-assigned this Mar 1, 2016

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Mar 3, 2016

Member

Looks like they got all built and installed fine. 😃

Will merge now.

Member

eXpl0it3r commented Mar 3, 2016

Looks like they got all built and installed fine. 😃

Will merge now.

@eXpl0it3r eXpl0it3r merged commit 77609e1 into master Mar 3, 2016

16 checks passed

debian-gcc-64 Build #93 done.
Details
freebsd-gcc-64 Build #93 done.
Details
osx-clang-universal Build #93 done.
Details
static-analysis Build #93 done.
Details
windows-gcc-471-tdm-32 Build #96 done.
Details
windows-gcc-471-tdm-64 Build #96 done.
Details
windows-gcc-481-tdm-32 Build #96 done.
Details
windows-gcc-481-tdm-64 Build #96 done.
Details
windows-gcc-520-mingw-32 Build #94 done.
Details
windows-gcc-520-mingw-64 Build #96 done.
Details
windows-vc11-32 Build #94 done.
Details
windows-vc11-64 Build #96 done.
Details
windows-vc12-32 Build #95 done.
Details
windows-vc12-64 Build #94 done.
Details
windows-vc14-32 Build #94 done.
Details
windows-vc14-64 Build #96 done.
Details

@eXpl0it3r eXpl0it3r deleted the feature/install_pdb branch Mar 3, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment