libassert icon indicating copy to clipboard operation
libassert copied to clipboard

[vcpkg] static usage fails

Open nickdademo opened this issue 1 year ago • 4 comments

Currently, in order to use libassert via vcpkg the following seems to be required:

find_package(cpptrace CONFIG REQUIRED)
set(assert_DIR ${VCPKG_INSTALLED_DIR}/${VCPKG_TARGET_TRIPLET}/share/libassert)
find_package(assert CONFIG REQUIRED)

The find_package(cpptrace CONFIG REQUIRED) is required if we are doing a static build of libassert (and perhaps cpptrace) - otherwise we get the following error:

CMake Error at build/dev-vs/vcpkg_installed/x64-windows/share/libassert/assert_targets.cmake:60 (set_target_properties):
  The link interface of target "assert::assert" contains:

    cpptrace::cpptrace

  but the target was not found.  Possible reasons include:

    * There is a typo in the target name.
    * A find_package call is missing for an IMPORTED target.
    * An ALIAS target is missing.

Call Stack (most recent call first):
  build/dev-vs/vcpkg_installed/x64-windows/share/libassert/assert-config.cmake:27 (include)
  vcpkg/scripts/buildsystems/vcpkg.cmake:859 (_find_package)
  CMakeLists.txt:64 (find_package)

Having to set assert_DIR is also a bit awkward IMO - this seems to be because the installed port directory libassert is different to the name of the package in the config filename (assert). Is this the best/only way? IMO the port should also install a usage file to state this requirement.

libassert port file modification to fix the first part could be by adding the following after vcpkg_cmake_config_fixup:

file(READ "${CURRENT_PACKAGES_DIR}/share/libassert/assert-config.cmake" LIBASSERT_CONFIG)
file(WRITE "${CURRENT_PACKAGES_DIR}/share/libassert/assert-config.cmake" "
include(CMakeFindDependencyMacro)
find_dependency(cpptrace)
${LIBASSERT_CONFIG}
")

nickdademo avatar Dec 09 '23 13:12 nickdademo

Thank you for the report, I'll look into this. You shouldn't have to set the assert_DIR. Did you install with the x64-windows-static triplet?

jeremy-rifkin avatar Dec 09 '23 20:12 jeremy-rifkin

Thank you for the report, I'll look into this. You shouldn't have to set the assert_DIR. Did you install with the x64-windows-static triplet?

Dynamic build but statically linking libassert and cpptrace via a triplet overlay.

nickdademo avatar Dec 10 '23 01:12 nickdademo

@jeremy-rifkin Can confirm, the fact that you name this library libassert but internally use assert for everything is causing conflicts where CMake, not even VCPKG can't even figure out where your library is, find_package is defined to look for [my-package-name]Config.cmake, what you expose as the name actually matters. This means the whole library is configured incorrectly, even ignoring the static lib part, you export one name, but then find_package a completely different one. All your CMakeLists.txt is saying "This library is called assert", but you effectively just expect it to work with a completely different name libassert. You could literally install your library with out vcpkg, and actually use it as find_package(assert).

CMake Error at external/vcpkg/scripts/buildsystems/vcpkg.cmake:859 (_find_package):
  Could not find a package configuration file provided by "libassert" with
  any of the following names:

    libassertConfig.cmake
    libassert-config.cmake

  Add the installation prefix of "libassert" to CMAKE_PREFIX_PATH or set
  "libassert_DIR" to a directory containing one of the above files.  If
  "libassert" provides a separate development package or SDK, be sure it has
  been installed.

Those files straight up don't exist, and the path is libassert/.... where as your cmake claims it should be assert/...

The easiest fix is to add a patch to vcpkg patch for the port that basically does what @nickdademo did, but that won't fix it for anybody but vcpkg users.

You have to change your cmake if you want to support find_package(libassert), but to be honest, it could be a pain to fix and potentially break things for everybody who uses this library as a submodule. IMO name conflicts and tools not being able to tell if your library is a CMake target or a literal .dll/.so file mean that it was a mistake to call this library either assert or libassert, generally you prefix it with some sort of namespace prefix (like rifkin-assert internally and as the library name for find_pacakge, exposed as an alias target rifkin::assert, provided this isn't the only library you maintain). But I digress.

Normally you would name the target in how you import it, you could for example use "libassert", internally everywhere, and then if you wanted to expose a different namespace you could do "assert::assert" and change the EXPORT_NAME target property to assert. You might be able to fix this by selectively changing different directories too, but this probably needs to be fixed from the root. You could call this whole thing assert, or you could keep libassert, and change the internal target to reflect that (keeping the alias target of assert::assert, and changing the export name for people who actually install this library) See this for more information on changing the export name https://stackoverflow.com/questions/71343475/how-to-have-separate-names-for-a-target-its-filename-and-its-export-name

so to recap, if you want to do the bigger change, you would change this:

if(ASSERT_STATIC)
  add_library(assert STATIC src/assert.cpp include/assert.hpp)
else()
  add_library(assert SHARED src/assert.cpp include/assert.hpp)
  set(CMAKE_POSITION_INDEPENDENT_CODE ON)
endif()

to this:

if(ASSERT_STATIC)
  add_library(libassert STATIC src/assert.cpp include/assert.hpp)
else()
  add_library(libassert SHARED src/assert.cpp include/assert.hpp)
  set(CMAKE_POSITION_INDEPENDENT_CODE ON)
endif()

cascade that name change down to the rest of your install stuff, including your config.cmake.in.

add this line:

add_library(assert::assert ALIAS libassert)

and then add

set_target_properties(
        libassert
        PROPERTIES
        EXPORT_NAME "assert"
)

and keep your namespaces

NAMESPACE assert::

This should allow subdirectory users and installed library users to see the assert::assert target, and keep this library named as libassert

Cazadorro avatar Dec 27 '23 18:12 Cazadorro

Thanks for the additional info. I’m somewhat skeptical of things breaking due to it being named assert, it hasn’t caused problems in any of my testing. The reason for naming it assert and not libassert is that the produced .a or .so or whatnot needs to not be called liblibassert, which would happen with add_library(libassert.

In the dev branch I’ve overhauled the cmake for the library, thanks to an awesome contributor, so the fixes there may end up fixing this. I’ll investigate more as well.

jeremy-rifkin avatar Dec 30 '23 18:12 jeremy-rifkin

Apologies for the delay here. I'm getting back to libassert stuff now and I'll resolve this in the upcoming release.

jeremy-rifkin avatar Feb 17 '24 16:02 jeremy-rifkin

@jeremy-rifkin Seems like everything is fixed by the new vcpkg port in https://github.com/microsoft/vcpkg/pull/36745

nickdademo avatar Feb 18 '24 04:02 nickdademo

Awesome thanks! Is set(assert_DIR ${VCPKG_INSTALLED_DIR}/${VCPKG_TARGET_TRIPLET}/share/libassert) still needed?

One way or another, I'm reworking the cmake to use libassert internally not assert for the reasons @Cazadorro pointed out :)

jeremy-rifkin avatar Feb 18 '24 18:02 jeremy-rifkin

Awesome thanks! Is set(assert_DIR ${VCPKG_INSTALLED_DIR}/${VCPKG_TARGET_TRIPLET}/share/libassert) still needed?

One way or another, I'm reworking the cmake to use libassert internally not assert for the reasons @Cazadorro pointed out :)

That line is no longer needed due to this change you made ;) https://github.com/microsoft/vcpkg/pull/36745/files#diff-1a798a1e0b8e4d7db49dbf3dcf570edb7bd081d0058b144ef4e06151db9d587fR24

nickdademo avatar Feb 19 '24 02:02 nickdademo

This is now resolved as as soon as https://github.com/microsoft/vcpkg/pull/37506 is merged

jeremy-rifkin avatar Mar 17 '24 04:03 jeremy-rifkin