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

Missing use of CMAKE_DEBUG_POSTFIX #2121

Closed
theirix opened this issue Nov 28, 2017 · 11 comments · Fixed by #2599
Closed

Missing use of CMAKE_DEBUG_POSTFIX #2121

theirix opened this issue Nov 28, 2017 · 11 comments · Fixed by #2599
Labels

Comments

@theirix
Copy link

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
Copy link
Member

bagder commented Dec 1, 2017

@snikulov or @jzakrzewski, comments?

@snikulov
Copy link
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
Copy link
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
Copy link

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
Copy link
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
Copy link

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
Copy link
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.

@theirix
Copy link
Author

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
Copy link
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.

@BeagleJoe
Copy link

@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
       using -DCMAKE_DEBUG_POSTFIX explicitly

       fixes curl#2121, obsoletes curl#2384
@snikulov
Copy link
Member

Will be fixed per #2599

snikulov added a commit that referenced this issue May 24, 2018
       using -DCMAKE_DEBUG_POSTFIX explicitly

       fixes #2121, obsoletes #2384
@lock lock bot locked as resolved and limited conversation to collaborators Aug 22, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Development

Successfully merging a pull request may close this issue.

8 participants