Skip to content

[dstorage] Fix port bug with RelWithDebInfo remap#37485

Closed
walbourn wants to merge 4 commits intomicrosoft:masterfrom
walbourn:dstoragefix
Closed

[dstorage] Fix port bug with RelWithDebInfo remap#37485
walbourn wants to merge 4 commits intomicrosoft:masterfrom
walbourn:dstoragefix

Conversation

@walbourn
Copy link
Member

@walbourn walbourn commented Mar 15, 2024

While working on #37401 I realized this port would not work correctly if I use set(CMAKE_MAP_IMPORTED_CONFIG_RELWITHDEBINFO Release) which is required for my other ports to work with RelWithDebInfo.

Also includes some cleanup for the port as well as respecting VCPKG_BUILD_TYPE=release.

@dg0yt
Copy link
Contributor

dg0yt commented Mar 16, 2024

While working on #37401 I realized this port would not work correctly if I use set(CMAKE_MAP_IMPORTED_CONFIG_RELWITHDEBINFO Release) which is required for my other ports to work with RelWithDebInfo.

Actually this doesn't need a change to the code but to that variable:

set(CMAKE_MAP_IMPORTED_CONFIG_RELWITHDEBINFO Release "") # or "Release;"

Cf.
#6393 (comment)
https://cmake.org/cmake/help/latest/prop_tgt/MAP_IMPORTED_CONFIG_CONFIG.html

Note that vcpkg already initializes the variable.

if(NOT DEFINED CMAKE_MAP_IMPORTED_CONFIG_RELWITHDEBINFO)
set(CMAKE_MAP_IMPORTED_CONFIG_RELWITHDEBINFO "RelWithDebInfo;Release;None;")
if(VCPKG_VERBOSE)
message(STATUS "VCPKG-Info: CMAKE_MAP_IMPORTED_CONFIG_RELWITHDEBINFO set to RelWithDebInfo;Release;None;")
endif()
endif()

@MonicaLiu0311 MonicaLiu0311 added the category:port-bug The issue is with a library, which is something the port should already support label Mar 18, 2024
Comment on lines +6 to +22
set_target_properties(Microsoft::DirectStorage PROPERTIES
IMPORTED_LOCATION "${_dstorage_root}/bin/dstorage.dll"
IMPORTED_IMPLIB "${_dstorage_root}/lib/@lib_name@"
INTERFACE_INCLUDE_DIRECTORIES "${_dstorage_root}/include"
INTERFACE_LINK_LIBRARIES "Microsoft::DirectStorageCore"
MAP_IMPORTED_CONFIG_MINSIZEREL ""
MAP_IMPORTED_CONFIG_RELWITHDEBINFO ""
IMPORTED_LINK_INTERFACE_LANGUAGES "C")

add_library(Microsoft::DirectStorageCore SHARED IMPORTED)
set_target_properties(Microsoft::DirectStorageCore PROPERTIES
IMPORTED_LOCATION "${_dstorage_root}/bin/dstoragecore.dll"
IMPORTED_IMPLIB "${_dstorage_root}/lib/@lib_name@"
INTERFACE_INCLUDE_DIRECTORIES "${_dstorage_root}/include"
MAP_IMPORTED_CONFIG_MINSIZEREL ""
MAP_IMPORTED_CONFIG_RELWITHDEBINFO ""
IMPORTED_LINK_INTERFACE_LANGUAGES "C")
Copy link
Contributor

@teo-tsirpanis teo-tsirpanis Mar 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you try if this will work?

Suggested change
set_target_properties(Microsoft::DirectStorage PROPERTIES
IMPORTED_LOCATION "${_dstorage_root}/bin/dstorage.dll"
IMPORTED_IMPLIB "${_dstorage_root}/lib/@lib_name@"
INTERFACE_INCLUDE_DIRECTORIES "${_dstorage_root}/include"
INTERFACE_LINK_LIBRARIES "Microsoft::DirectStorageCore"
MAP_IMPORTED_CONFIG_MINSIZEREL ""
MAP_IMPORTED_CONFIG_RELWITHDEBINFO ""
IMPORTED_LINK_INTERFACE_LANGUAGES "C")
add_library(Microsoft::DirectStorageCore SHARED IMPORTED)
set_target_properties(Microsoft::DirectStorageCore PROPERTIES
IMPORTED_LOCATION "${_dstorage_root}/bin/dstoragecore.dll"
IMPORTED_IMPLIB "${_dstorage_root}/lib/@lib_name@"
INTERFACE_INCLUDE_DIRECTORIES "${_dstorage_root}/include"
MAP_IMPORTED_CONFIG_MINSIZEREL ""
MAP_IMPORTED_CONFIG_RELWITHDEBINFO ""
IMPORTED_LINK_INTERFACE_LANGUAGES "C")
set_target_properties(Microsoft::DirectStorage PROPERTIES
IMPORTED_CONFIGURATIONS "RELEASE"
IMPORTED_LOCATION_RELEASE "${_dstorage_root}/bin/dstorage.dll"
IMPORTED_IMPLIB_RELEASE "${_dstorage_root}/lib/@lib_name@"
INTERFACE_INCLUDE_DIRECTORIES "${_dstorage_root}/include"
INTERFACE_LINK_LIBRARIES "Microsoft::DirectStorageCore"
IMPORTED_LINK_INTERFACE_LANGUAGES "C")
add_library(Microsoft::DirectStorageCore SHARED IMPORTED)
set_target_properties(Microsoft::DirectStorageCore PROPERTIES
IMPORTED_CONFIGURATIONS "RELEASE"
IMPORTED_LOCATION_RELEASE "${_dstorage_root}/bin/dstoragecore.dll"
IMPORTED_IMPLIB_RELEASE "${_dstorage_root}/lib/@lib_name@"
INTERFACE_INCLUDE_DIRECTORIES "${_dstorage_root}/include"
IMPORTED_LINK_INTERFACE_LANGUAGES "C")

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works but ONLY if you have set(CMAKE_MAP_IMPORTED_CONFIG_RELWITHDEBINFO "Release")

The 'correct' solution for CMake's perspective seems to me to be (a) use the non-config specific versions of the library, but be sure to set all the remap properties to empty.

Comment on lines +23 to +27

if(NOT DEFINED VCPKG_BUILD_TYPE)
file(MAKE_DIRECTORY "${CURRENT_PACKAGES_DIR}/debug")
file(INSTALL "${CURRENT_PACKAGES_DIR}/bin" "${CURRENT_PACKAGES_DIR}/lib" DESTINATION "${CURRENT_PACKAGES_DIR}/debug")
endif()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If my suggestion above works, can you try removing this?

@walbourn
Copy link
Member Author

While working on #37401 I realized this port would not work correctly if I use set(CMAKE_MAP_IMPORTED_CONFIG_RELWITHDEBINFO Release) which is required for my other ports to work with RelWithDebInfo.

Actually this doesn't need a change to the code but to that variable:

set(CMAKE_MAP_IMPORTED_CONFIG_RELWITHDEBINFO Release "") # or "Release;"

Cf. #6393 (comment) https://cmake.org/cmake/help/latest/prop_tgt/MAP_IMPORTED_CONFIG_CONFIG.html

Note that vcpkg already initializes the variable.

if(NOT DEFINED CMAKE_MAP_IMPORTED_CONFIG_RELWITHDEBINFO)
set(CMAKE_MAP_IMPORTED_CONFIG_RELWITHDEBINFO "RelWithDebInfo;Release;None;")
if(VCPKG_VERBOSE)
message(STATUS "VCPKG-Info: CMAKE_MAP_IMPORTED_CONFIG_RELWITHDEBINFO set to RelWithDebInfo;Release;None;")
endif()
endif()

But this change was only made VERY recently I see...

@Neumann-A
Copy link
Contributor

But this change was only made VERY recently I see...

The only thing that is new there is None. The variable has been set for years....

@walbourn
Copy link
Member Author

Looks like none of this is actually needed thanks to #35940 removing the need for my CMakeLists to use CMAKE_MAP_IMPORTED_CONFIG_RELWITHDEBINFO at all.

@walbourn walbourn closed this Mar 19, 2024
@walbourn walbourn deleted the dstoragefix branch March 21, 2024 17:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

category:port-bug The issue is with a library, which is something the port should already support

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants