nemtrif / utfcpp

UTF-8 with C++ in a Portable Way
Boost Software License 1.0
1.57k stars 201 forks source link

After 4.0.0 headers gets installed into include/utf8 using CMake #112

Closed diizzyy closed 11 months ago

diizzyy commented 1 year ago

That doesn't seem to be intentional by looking at the rest of the source code?

nemtrif commented 1 year ago

It gets installed directly under include:

diizzyy commented 1 year ago

Before 4.0.0

include/utf8cpp/utf8.h
include/utf8cpp/utf8/checked.h
include/utf8cpp/utf8/core.h
include/utf8cpp/utf8/cpp11.h
include/utf8cpp/utf8/cpp17.h
include/utf8cpp/utf8/unchecked.h

After:

include/utf8.h
include/utf8/checked.h
include/utf8/core.h
include/utf8/cpp11.h
include/utf8/cpp17.h
include/utf8/cpp20.h
include/utf8/unchecked.h

Is that change intentional? It also seems a bit confusing as the rest of the project is referred to as utf8cpp?

cmake/utf8cppTargets.cmake (in 4.0.1)

...
set_target_properties(utf8cpp::utf8cpp PROPERTIES
  INTERFACE_INCLUDE_DIRECTORIES "${_IMPORT_PREFIX}/include/utf8cpp"

There are also distros reverting this change due to breakage with consumers

ufleisch commented 1 year ago

Unfortunately, the headers are not only installed into a different folder, the CMake build system is broken. If this gets unnoticed, it is probably because utf8.h gets installed into the standard include directory, so it is found anyway, even if the dependency to utfcpp is not correctly specified. To see the issue, we need to use a non-standard include path, e.g. using a custom prefix. This is not a theoretical problem, currently, TagLib cannot be built without hacks on macOS using Homebrew because of this.

It can be reproduced like this, first it is shown that it works correctly for version 3.2.5

git clone git@github.com:nemtrif/utfcpp.git
cd utfcpp
git checkout v3.2.5
cmake -B build -DCMAKE_INSTALL_PREFIX=$HOME/custom-prefix -DUTF8_TESTS=OFF
make -C build
make -C build install

mkdir ../utfcpp-client
cd ../utfcpp-client
cat >CMakeLists.txt <<EOF
cmake_minimum_required(VERSION 3.5.0)
project(utfcpp-client)
find_package(utf8cpp)
add_executable(client client.cpp)
target_link_libraries(client utf8::cpp)
EOF
cat >client.cpp <<EOF
#include <string>
#include <iostream>
#include <utf8.h>
int main(int argc, char *argv[]) {
  std::u16string utf16string = {0x41, 0x0448, 0x65e5, 0xd834, 0xdd1e};
  std::string u = utf8::utf16to8(utf16string);
  std::cout << u << std::endl;
  return 0;
}
EOF
cmake -B build -DCMAKE_PREFIX_PATH=$HOME/custom-prefix
make -C build
# Run build/client, it works

Now the same with 4.0.1

cd ../utfcpp
rm -rf $HOME/custom-prefix build
git checkout v4.0.1
cmake -B build -DCMAKE_INSTALL_PREFIX=$HOME/custom-prefix -DUTF8_TESTS=OFF
make -C build
make -C build install

cd ../utfcpp-client
cmake -B build -DCMAKE_PREFIX_PATH=$HOME/custom-prefix

-- Configuring done CMake Error at CMakeLists.txt:5 (target_link_libraries): Target "client" links to:

utf8::cpp

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.

Looking into $HOME/custom-prefix/share/utf8cpp/cmake/utf8cppTargets.cmake one sees that the imported target is now utf8cpp::utf8cpp and no longer utf8::cpp (as set previously in utf8cppConfig.cmake). So let's change this.

sed -i 's/utf8::cpp/utf8cpp::utf8cpp/' CMakeLists.txt

cmake -B build -DCMAKE_PREFIX_PATH=$HOME/custom-prefix

-- Configuring done CMake Error in CMakeLists.txt: Imported target "utf8cpp::utf8cpp" includes non-existent path

"/home/urs/custom-prefix/include/utf8cpp"

in its INTERFACE_INCLUDE_DIRECTORIES. Possible reasons include:

  • The path was deleted, renamed, or moved to another location.

  • An install or uninstall procedure did not complete successfully.

  • The installation package was faulty and references files it does not provide.

Another possibility would be to use utf8cpp as the name of the imported CMake target. But this will only work with v3.2.5, with v4.0.1 we get

(..)/utfcpp-client/client.cpp:3:10: fatal error: utf8.h: No such file or directory 3 | #include

I would suggest the following changes to fix it:

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 6fdd906..2ca50a6 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -10,7 +10,7 @@ include(GNUInstallDirs)

 target_include_directories(utf8cpp INTERFACE
     "$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/source>"
-    $<INSTALL_INTERFACE:include/utf8cpp>
+    $<INSTALL_INTERFACE:include>
 )

 include(CMakePackageConfigHelpers)
diff --git a/utf8cppConfig.cmake.in b/utf8cppConfig.cmake.in
index 9c15f36..4bdb9c4 100644
--- a/utf8cppConfig.cmake.in
+++ b/utf8cppConfig.cmake.in
@@ -2,3 +2,7 @@

 include("${CMAKE_CURRENT_LIST_DIR}/@PROJECT_NAME@Targets.cmake")
 check_required_components("@PROJECT_NAME@")
+
+if(NOT TARGET utf8::cpp)
+    add_library(utf8::cpp ALIAS utf8cpp::utf8cpp)
+endif()
nemtrif commented 1 year ago

Fixed by @ufleisch, I think.

diizzyy commented 1 year ago

@ufleisch Shouldn't we also use the more common lib/cmake/projectname install path rather than installing directly into share/cmake ?

mhx commented 11 months ago

Fixed by @ufleisch, I think.

I don't think this is actually fixed as the files are still installed to a different location as before.

I haven't found a rationale anywhere for why the files were moved from /usr/include/utf8cpp to /usr/include in the first place. The only hint is a "Rewrite CMakeLists" note in 925e7147, but that doesn't even indicate that move was intentional.

nemtrif commented 11 months ago

Hopefully fixed in 4.0.3