Missing use of CMAKE_DEBUG_POSTFIX #2121

Closed
theirix opened this Issue Nov 28, 2017 · 11 comments

Comments

Projects
None yet
8 participants
@theirix

theirix commented Nov 28, 2017

I did this

Build a latest 7.56.1 curl on Windows with Debug configuration and
cmake setting CMAKE_DEBUG_POSTFIX="" to avoid a -d suffix for library files.

I expected the following

I except fo find a libcurl_imp.lib file instead of libcurl-d_imp.lib.

I think the reason is that a line

DEBUG_POSTFIX "-d"
does not use the CMAKE_DEBUG_POSTFIX variable from the topmost cmakefile.

Both 7.56.1 and master branch contain this line.

curl/libcurl version

7.56.1

operating system

Windows, Visual Studio 2015, x86

@bagder bagder added the cmake label Nov 29, 2017

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Dec 1, 2017

Member

@snikulov or @jzakrzewski, comments?

Member

bagder commented Dec 1, 2017

@snikulov or @jzakrzewski, comments?

@snikulov

This comment has been minimized.

Show comment
Hide comment
@snikulov

snikulov Dec 1, 2017

Member

@bagder Depends on naming conventions you want to use for library name.
For example, currently, if you'll use winbuild https://github.com/curl/curl/blob/master/winbuild/Makefile.vc#L170
it will be either libcurl-debug/libcurl-release and not -d for debug as @paulharris introduced in #1649

set_target_properties (${LIB_NAME} PROPERTIES DEBUG_POSTFIX "-d"....

will definitely enforce exact postfix for name without any customization possible.
Otherwise CMAKE_<CONFIG>_POSTFIX provides ability to change this property from command line.

IMO, for Windows sometimes it is good to have information in which mode it was built.

Member

snikulov commented Dec 1, 2017

@bagder Depends on naming conventions you want to use for library name.
For example, currently, if you'll use winbuild https://github.com/curl/curl/blob/master/winbuild/Makefile.vc#L170
it will be either libcurl-debug/libcurl-release and not -d for debug as @paulharris introduced in #1649

set_target_properties (${LIB_NAME} PROPERTIES DEBUG_POSTFIX "-d"....

will definitely enforce exact postfix for name without any customization possible.
Otherwise CMAKE_<CONFIG>_POSTFIX provides ability to change this property from command line.

IMO, for Windows sometimes it is good to have information in which mode it was built.

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Dec 2, 2017

Member

I don't personally build either on windows nor with cmake which is why I'm asking for input. If nobody has any opinions which way is the best, the most common way or just works out better in practice, then I'm fine with closing this but it feels like our current way has landed without a lot of thought on these details...

Member

bagder commented Dec 2, 2017

I don't personally build either on windows nor with cmake which is why I'm asking for input. If nobody has any opinions which way is the best, the most common way or just works out better in practice, then I'm fine with closing this but it feels like our current way has landed without a lot of thought on these details...

@paulharris

This comment has been minimized.

Show comment
Hide comment
@paulharris

paulharris Dec 4, 2017

I have an opinion, and I have put thought into it, based on my experiences of debugging debug-release mismatched library link issues.

@theirix Please read the details in my #1649 pull.
You really need to ensure you link to the right library on Windows, so it is typical to add postfixes to ensure the correct runtimes are always linked in. It might be better in the long run for your project if you ensure you link in the debug-postfix-d library for debug builds, and no-postfix library for release builds.
This is how CMake projects typically work, however there IS a lot of variation and confusion in this area.

Having said that, I don't see any problem with being able to override via CMAKE_DEBUG_POSTFIX.

I imagine we'd put something like this at the top of CMakeLists.txt

if(NOT CMAKE_DEBUG_POSTFIX)
set(CMAKE_DEBUG_POSTFIX "-d")
endif()

And then adjust the below to:
set_target_properties (${LIB_NAME} PROPERTIES
DEBUG_POSTFIX ${CMAKE_DEBUG_POSTFIX}

I would suggest that the default behaviour is the safest - use postfixes in the filenames.
Mismatched debug/release library builds are a pain in the bum on Windows.
Make mistakes hard to make.

Oh, and I chose "-d" instead of "-debug" as I was following other conventions from other libraries. There are probably 3 different naming conventions commonly seen, but -debug and -release is not one of the more common ones I've come across.

cheers,
Paul

paulharris commented Dec 4, 2017

I have an opinion, and I have put thought into it, based on my experiences of debugging debug-release mismatched library link issues.

@theirix Please read the details in my #1649 pull.
You really need to ensure you link to the right library on Windows, so it is typical to add postfixes to ensure the correct runtimes are always linked in. It might be better in the long run for your project if you ensure you link in the debug-postfix-d library for debug builds, and no-postfix library for release builds.
This is how CMake projects typically work, however there IS a lot of variation and confusion in this area.

Having said that, I don't see any problem with being able to override via CMAKE_DEBUG_POSTFIX.

I imagine we'd put something like this at the top of CMakeLists.txt

if(NOT CMAKE_DEBUG_POSTFIX)
set(CMAKE_DEBUG_POSTFIX "-d")
endif()

And then adjust the below to:
set_target_properties (${LIB_NAME} PROPERTIES
DEBUG_POSTFIX ${CMAKE_DEBUG_POSTFIX}

I would suggest that the default behaviour is the safest - use postfixes in the filenames.
Mismatched debug/release library builds are a pain in the bum on Windows.
Make mistakes hard to make.

Oh, and I chose "-d" instead of "-debug" as I was following other conventions from other libraries. There are probably 3 different naming conventions commonly seen, but -debug and -release is not one of the more common ones I've come across.

cheers,
Paul

@jay

This comment has been minimized.

Show comment
Hide comment
@jay

jay Dec 4, 2017

Member

We have a different issue at #1857 that I have a related question about. We are thinking about making the debug postfix uniform across builds, so that winbuild uses d (instead of _debug) and cmake uses d (instead of -d). Visual Studio project files already use d like curld.exe and libcurld.dll. Do you think we should leave them alone or make them uniform? I thought it would be easier to make them uniform but now I'm not sure.

Member

jay commented Dec 4, 2017

We have a different issue at #1857 that I have a related question about. We are thinking about making the debug postfix uniform across builds, so that winbuild uses d (instead of _debug) and cmake uses d (instead of -d). Visual Studio project files already use d like curld.exe and libcurld.dll. Do you think we should leave them alone or make them uniform? I thought it would be easier to make them uniform but now I'm not sure.

@paulharris

This comment has been minimized.

Show comment
Hide comment
@paulharris

paulharris Dec 4, 2017

I personally prefer -d to d because I prefer the variant / versions to be separate from the project name, not mashed together (ie becoming curld).
For the ultimate (excessive?) example, see boost's naming conventions...
libboost_signals-gcc55-mt-d-1_65_0.so
Long winded but really hard to mixed up build variants.

But name would be much worse if it were
libboost_signalsgcc55mtd1650

-debug is not too bad, but its just longer than it needs to be.
Everyone seems to know -d means debug.

I personally prefer -d to d because I prefer the variant / versions to be separate from the project name, not mashed together (ie becoming curld).
For the ultimate (excessive?) example, see boost's naming conventions...
libboost_signals-gcc55-mt-d-1_65_0.so
Long winded but really hard to mixed up build variants.

But name would be much worse if it were
libboost_signalsgcc55mtd1650

-debug is not too bad, but its just longer than it needs to be.
Everyone seems to know -d means debug.

@jzakrzewski

This comment has been minimized.

Show comment
Hide comment
@jzakrzewski

jzakrzewski Dec 4, 2017

Contributor

@paulharris Everyone has his/her preferences to the naming. I for once prefer simply "d". But above all they should be uniform. It's a pain in the back when one has to depend on the way the lib has been build to get the right suffix in some scripts. This is also one of the reasons I have started with #1857 in the first place.

As for the way the postfix is set, I agree with @snikulov - it shouldn't be hardcoded with target properties but use the standard CMake variable.

Contributor

jzakrzewski commented Dec 4, 2017

@paulharris Everyone has his/her preferences to the naming. I for once prefer simply "d". But above all they should be uniform. It's a pain in the back when one has to depend on the way the lib has been build to get the right suffix in some scripts. This is also one of the reasons I have started with #1857 in the first place.

As for the way the postfix is set, I agree with @snikulov - it shouldn't be hardcoded with target properties but use the standard CMake variable.

@theirix

This comment has been minimized.

Show comment
Hide comment
@theirix

theirix Dec 4, 2017

Having said that, I don't see any problem with being able to override via CMAKE_DEBUG_POSTFIX.

I imagine we'd put something like this at the top of CMakeLists.txt

if(NOT CMAKE_DEBUG_POSTFIX)
set(CMAKE_DEBUG_POSTFIX "-d")
endif()

Of course I understand that VS builds prefer to use different suffices for debug and release builds. It's ok if the build have a default suffix (-d or d). We should only have a possibility to override it if we want to. Setting non-empty CMAKE_DEBUG_POSTFIX as in the snippet above should solve the problem.

theirix commented Dec 4, 2017

Having said that, I don't see any problem with being able to override via CMAKE_DEBUG_POSTFIX.

I imagine we'd put something like this at the top of CMakeLists.txt

if(NOT CMAKE_DEBUG_POSTFIX)
set(CMAKE_DEBUG_POSTFIX "-d")
endif()

Of course I understand that VS builds prefer to use different suffices for debug and release builds. It's ok if the build have a default suffix (-d or d). We should only have a possibility to override it if we want to. Setting non-empty CMAKE_DEBUG_POSTFIX as in the snippet above should solve the problem.

@bradking

This comment has been minimized.

Show comment
Hide comment
@bradking

bradking Mar 19, 2018

Contributor

Using

if(NOT DEFINED CMAKE_DEBUG_POSTFIX)
  set(CMAKE_DEBUG_POSTFIX "-d")
endif()

along with dropping the explicit DEBUG_POSTFIX property setting should be enough. See also #2384.

Contributor

bradking commented Mar 19, 2018

Using

if(NOT DEFINED CMAKE_DEBUG_POSTFIX)
  set(CMAKE_DEBUG_POSTFIX "-d")
endif()

along with dropping the explicit DEBUG_POSTFIX property setting should be enough. See also #2384.

@BeagleJoe

This comment has been minimized.

Show comment
Hide comment
@BeagleJoe

BeagleJoe Mar 19, 2018

@bradking Thanks. That should work. For my use, I want to set CMAKE_DEBUG_POSTFIX on the command line. (on Windows)

@bradking Thanks. That should work. For my use, I want to set CMAKE_DEBUG_POSTFIX on the command line. (on Windows)

snikulov added a commit to snikulov/curl that referenced this issue May 23, 2018

cmake: set -d postfix for debug builds if not specified
       using -DCMAKE_DEBUG_POSTFIX explicitly

       fixes #2121, obsoletes #2384
@snikulov

This comment has been minimized.

Show comment
Hide comment
@snikulov

snikulov May 23, 2018

Member

Will be fixed per #2599

Member

snikulov commented May 23, 2018

Will be fixed per #2599

@snikulov snikulov closed this in #2599 May 24, 2018

snikulov added a commit that referenced this issue May 24, 2018

cmake: set -d postfix for debug builds if not specified
       using -DCMAKE_DEBUG_POSTFIX explicitly

       fixes #2121, obsoletes #2384
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment