-
-
Notifications
You must be signed in to change notification settings - Fork 298
allow to modify or remove the postfix for debug builds #5600
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
allow to modify or remove the postfix for debug builds #5600
Conversation
Needs a release note, at the very least |
I don't like this change as is - first the default can be overridden easiy; Second it is altering a setting that could be relied on due to it being a long standing setting. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is not the way to do this and why just for NOT win32.
The default should be to do what other libraries do and not have a suffix. Anyone who cares about this can change it, but I think most people don't want to change linker commands when building a debug library. Windows is different since the debug library will link to the debug CRT. |
As for "long standing settings", 2.0 is the time to do the right thing instead of the traditional thing. Also, this isn't this only a long-standing thing on CMake? |
config/HDFMacros.cmake
Outdated
@@ -476,8 +476,6 @@ macro (HDF_DIR_PATHS package_prefix) | |||
if(NOT CMAKE_DEBUG_POSTFIX) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would add an option here "ENABLE_DEBUG_NAME_EXT" that is normally OFF. That is what this code was supposed to accomplish, just to make it easy and optional.
This way one does not need to remember that if I want an extension that I need to set it on Linux but skip WIN32. Easier to change one setting without regard to platform.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about making CMAKE_DEBUG_POSTFIX
accesible via the commandline, i.e.-DCMAKE_DEBUG_POSTFIX=
?
We should just move these settings (the whole if-else) to the cacheinit.cmake file as that is the intended use for that file. |
563e373
to
93f1e55
Compare
config/cmake/cacheinit.cmake
Outdated
@@ -40,6 +40,8 @@ set (HDF_TEST_EXPRESS "2" CACHE STRING "Control testing framework (0-3)" FORCE) | |||
|
|||
set (HDF5_MINGW_STATIC_GCC_LIBS ON CACHE BOOL "Statically link libgcc/libstdc++" FORCE) | |||
|
|||
set (CMAKE_DEBUG_POSTFIX "" CACHE STRING "Filename postfix for debug builds" FORCE) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just replace this with the whole thing:
if(NOT CMAKE_DEBUG_POSTFIX)
if (WIN32)
set (CMAKE_DEBUG_POSTFIX "_D")
else ()
set (CMAKE_DEBUG_POSTFIX "_debug")
endif ()
endif ()
93f1e55
to
840438c
Compare
@byrnHDF Does this handle setting of a postfix for developer builds of the library? |
Yes this affects the $CONFIG:Developer>: generator instances |
840438c
to
20670fa
Compare
Tested locally, works as expected. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs a release note - probably 2 or 3 lines, one to describe the problem, one to describe the change, and one saying Fixes or Fixed GitHub issue #4647. This should be added to the top of the Bug Fixes / Configuration section at ~ line 743 of release_docs/RELEASE.txt file.
The cause of the netCDF test failures is known and unrelated to this pull request.
20670fa
to
296205b
Compare
release_docs/RELEASE.txt
Outdated
@@ -742,6 +742,11 @@ Bug Fixes since HDF5-2.0.0 release | |||
|
|||
Configuration | |||
------------- | |||
- Allow to set postfix of target file names for debug builds |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove default setting of CMAKE_DEBUG_POSTFIX if not set.
Move the default setting of CMAKE_DEBUG_POSTFIX to the cacheinit,cmake file usually used by testing. If CMAKE_DEBUG_POSTFIX is not set with a -D option then CMAKE_DEBUG_POSTFIX will be the default provided by CMake itself,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find "Remove default setting of CMAKE_DEBUG_POSTFIX if not set." misleading. cacheinit.cmake
is part of the release (at least the current), so when building HDF5 the old behavior of having postfixes for debug build is restored.
There is also no such thing as a default provided by CMake.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove default setting of CMAKE_DEBUG_POSTFIX if not set.
yes should just be:
Remove default setting of CMAKE_DEBUG_POSTFIX
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cacheinit.cmake is provided just for testing and convenience and should only be used if the settings agrree with your expectations. The file can be usedas is with overrides or editing. There are many alternatives. The preferred method now is to use Presets.
This change affects the preferred Presets method by removing the settings. The binaries we build for publication (which do not build debug) use the Presets method. Future changes will be to eliminate the cacheinit.cmake file for a "Testing" CMakeUsersPresets.json file that we would create and use.
Since the default CMake setting for this CMAKE_DEBUG_POSTFIX isn't implemented the result is that it is the empty string, which is what we want.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change release note
Move the default setting of CMAKE_DEBUG_POSTFIX to the cacheinit.cmake file usually used by testing. If CMAKE_DEBUG_POSTFIX is not set with a -D option then CMAKE_DEBUG_POSTFIX will be the default provided by CMake itself.
296205b
to
345a9f5
Compare
thanks for all the support! |
closes #4647