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

Install on windows fixes #1117

Merged
merged 2 commits into from
Sep 27, 2022
Merged

Conversation

bjosv
Copy link
Contributor

@bjosv bjosv commented Sep 15, 2022

After running a temporary changed CI job (build.yaml, job windows)
that builds and installs but using: ninja -v all install and a matrix of CMAKE_BUILD_TYPE
the following changes were found.

  1. The pdb file is not longer created for hiredis_static since #1054, so remove installation attempts of the file on build types Debug or RelWithDebInfo
  2. Copy the solution in #1054 for hiredis_ssl_static

From ff57c18 (redis#1054) the debug information was embedded in the
windows static lib rather than creating a .pdb file.
Removing the installation step of this file in CMake when building
with buildtype Debug or RelWithDebInfo.
See ff57c18 (redis#1054) for same change in library hiredis_static.
@bjosv bjosv mentioned this pull request Sep 15, 2022
@bjosv
Copy link
Contributor Author

bjosv commented Sep 15, 2022

One observation is that the debug information is added to the static library indifferent of the build type, i.e even for Release builds. Previously the .pdb debug file were only installed when building with the CMake build type Debug or RelWithDebInfo.

@kristjanvalur I guess you know Windows best, is this the common way? or should it be changed to only include debug info in Debug/RelWithDebInfo builds? I think that CMake only adds debug flags in Linux when building for Debug/RelWithDebInfo (like the -g flag).

@kristjanvalur
Copy link
Contributor

Hi, sorry for late reply.
The presence of debug info in the .lib file has nothing to do with what happens during final linking. During linking of a .dll or .exe a .pdb may be generated. The necessary data can be found in the .lib file, or ignored. This change only means that an extra sidecar file is not needed to be lugged around with the static lib if you want debug info in the final.

@bjosv
Copy link
Contributor Author

bjosv commented Sep 27, 2022

Thanks @kristjanvalur. This is fine with me.

With this PR we now make sure that both installed static libraries can provide debug info in the same way, indifferent of CMake's build type.

@michael-grunder michael-grunder merged commit 6d5c3ee into redis:master Sep 27, 2022
@bjosv bjosv deleted the windows-cmake-fixes branch September 28, 2022 06:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants