search

arvidn / libtorrent

an efficient feature complete C++ bittorrent implementation
http://libtorrent.org
Other
5.25k stars 996 forks source link

Boost target_include_directories should probably be PRIVATE #6464

Closed elelel closed 2 years ago

elelel commented 3 years ago

Please provide the following information

libtorrent version (or branch): master

platform/architecture: Cross-compiling from x86-64 host to various targets

compiler and compiler version: Clang

When compiling as CMake subdirectory (through add_subdriectory) with custom Boost location set through CMake's FindBoost variables, if that location is within the project source, the following error will occur:

CMake Error in /mnt/project/src/contrib/libtorrent/CMakeLists.txt:
  Target "torrent-rasterbar" INTERFACE_INCLUDE_DIRECTORIES property contains
  path:

    "/mnt/project/src/main/cpp/../../../include.built/boost/x86_64"

  which is prefixed in the source directory.

CMake Generate step failed.  Build files cannot be regenerated correctly.

This can be solved by changing PUBLIC to PRIVATE in this line: https://github.com/arvidn/libtorrent/blob/362366481ee567c23e29bf0495ee640d3d66af4a/CMakeLists.txt#L795

Was there any special reason why it was declared PUBLIC there?

arvidn commented 3 years ago

I believe there are libtorrent header files that include boost headers, which means clients of libtorrent also need to make the boost include path available

elelel commented 3 years ago

Do I understand it correctly, that it was done to save the client's CMakeLists.txt from explicitly adding Boost include directories with it's own target_include_directories directive? And libtorrent is just not supposed to be used with Boost headers and libraries if they are located in project's source tree?

arvidn commented 3 years ago

Do I understand it correctly, that it was done to save the client's CMakeLists.txt from explicitly adding Boost include directories with it's own target_include_directories directive?

Yes. Just like a client of any library should inherit any configuration from the target that's required to link against it. libtorrent requires that clients of it also have boost includes available, so it must be propagated to clients.

And libtorrent is just not supposed to be used with Boost headers and libraries if they are located in project's source tree?

I don't see how that follows.

If you're suggesting that libtorrent can be built with one boost installation and then used with another, that's certainly not ok. They must be the same.

elelel commented 3 years ago

I don't see how that follows.

If you're suggesting that libtorrent can be built with one boost installation and then used with another, that's certainly not ok. They must be the same.

I'm trying to use the same boost for my target and libtorrent, but which is located under project's source tree. Let me explain it in more details how I see it. CMake's target_include_directories if called with PUBLIC will add the argument to INTERFACE_INCLUDE_DIRECTORIES variable. That variable is used by CMake with assumption of portability. In particular, when it generates install scripts, it uses these paths. Since people generally misunderstand that consequence, CMake developers added a safeguard to prevent execution of CMake scripts if such public path is obviously not portable. One case of such non-portable path is the source directory of the project being compiled, if CMake sees that such path is instructed to be reexported as a public (install) location dependency it can know for sure that it's wrong and prevents compilation. In other cases such a check can't be done, and the compilation will continue. My project's source tree contains precompiled boost libraries for different architectures for cross-compiling. Both my project and libtorrent compiled use the same boost library each time. The problem is that libtorrent's CMakeLists.txt is trying to reexport Boost's location, which it obtained from FindBoost as a public include. The thing is that there's no reason to assume that such path is a public-exportable location. In fact various FindBoost hint variables are made specifically to hint to search for Boost at any custom location, which is, most likely is not re-exportable. I can easily fool CMake's safeguard check by copying Boost to some non-source prefixed directory, to work that around. But I think that assumptions made in libtorrent's CMakeLists are wrong, because using an internal (project-local) Boost is quite legitimate scenario.

arvidn commented 3 years ago

I'm not a cmake expert, but I don't understand what it means for an include path to "non-portable". It's a path on your computer that your compiler can use as an include path, what's not portable about it?

It's possible that I've misunderstood what PUBLIC means, but if it doesn't mean what I think it does, is there something in cmake that does? Just removing the PUBLIC keyword alone isn't a fix, because it will leave the libtorrent cmake target broken. A client can no longer rely on cmake to ensure it can link against that target.

elelel commented 3 years ago

By portable I mean that such an include path is always valid, e.g. when you're doing a 'make install'. This stackoverflow question probably does a better job explaining it than me: https://stackoverflow.com/questions/25676277/cmake-target-include-directories-prints-an-error-when-i-try-to-add-the-source

Note that they're suggesting to use generator expression to distinguish the path for build and install interface. But in our case we can't do that, because a portable path (for install interface) may just not exist. The path obtained from FindBoost is not necessarily a path that is valid for install targets. When I'm using libtorrent through add_subdirectory I'm not planning to do an "install" at all, I just link to the library, but CMake can't know that. So it assumes the install target must be generated. And that install target is checked for the validity of exported paths. There is almost no way for CMake to make such checks, but an absolute path to a source directory is obviously is not that is valid for "install". It doesn't mean that any other arbitrary is valid, though, CMake just doesn't have a way to verify the path, so it will compile without a glitch generating a possibly invalid install target.

As for relying on CMake, it can't do that currently either, at least in add_subdirectory case. Pushing an include path to parent doesn't lead to any predictable result, the parent may do whatever it wants within it's CMakeLists with include directories. It may already include different Boost in include and you try to override it, or it may add a target_include_dir with it's own after calling add_subdirectory with your CMakeLists.txt. The result is not predictable.

Here's a lengthy post with an explanation PUBLIC/PRIVATE pitfalls if you are interested: https://crascit.com/2016/01/31/enhanced-source-file-handling-with-target_sources/ In particular:

The PRIVATE and PUBLIC keywords specify where those corresponding sources should be used. PRIVATE simply means those sources should only be added to myLib, whereas PUBLIC means those sources should be added to myLib and to any target that links to myLib. An INTERFACE keyword can be used for sources that should not be added to myLib but should be added to anything that links to myLib. In practice, sources will almost always be PRIVATE, since they shouldn’t generally be added to anything that links against the target. Header-only interface libraries are one exception because sources can only be added as INTERFACE for interface libraries. Do not confuse the PRIVATE , PUBLIC and INTERFACE keywords with whether a header is part of the public API for the library or not, the keywords are specifically for controlling which target(s) the sources are added to in this case. ... Lastly, the target_include_directories() command adds the foo subdirectory to the header search path for both myLib and anything linking to it. Therefore, any other source file in another directory that needs to #include the foo.h header will also be able to find it.

FranciscoPombal commented 3 years ago

@arvidn @elelel

I believe the issue here stems from incorrect dependency propagation due to the use of ${Boost_INCLUDE_DIRS} and such variables instead of relying on the propagation guarantees afforded by the use of targets, which is the best practice in modern CMake.

Among other things, https://github.com/arvidn/libtorrent/pull/6270 is supposed to solve such problems.

To be fair, I have not tested it against OP's exact case ("compiling as CMake subdirectory (through add_subdriectory) with custom Boost location [within the project source] set through CMake's FindBoost variables"), but I would expect it to be robust enough to work correctly in that case as well.

@elelel https://github.com/arvidn/libtorrent/pull/6270 got stuck in discussion hell, but perhaps you could give it a try and see if it solves your issue.

arvidn commented 3 years ago

@FranciscoPombal the libtorrent target links against the boost target, so one would expect that libtorrent itself has the boost include directory visible to it. However, the libtorrent target must also propagate this include directory to its clients. I would expect this to be done by declaring the dependency on boost to be PUBLIC.

I suppose this ticket is about messing with the include directories directly, and explicitly, which I suppose is questionable.

FranciscoPombal commented 3 years ago

@arvidn

I suppose this ticket is about messing with the include directories directly, and explicitly, which I suppose is questionable.

"Messing with the directories directly and explicitly" is exactly what the current CML does by using the ${Boost_INCLUDE_DIRS} variable in the target_* call. I believe OP is running into issues with paths not being correctly resolved/adapted when propagated, which results from using such variables like this, and I believe using the target abstractions, as done in https://github.com/arvidn/libtorrent/pull/6270, solves this problem (i.e., it results in CMake correctly resolving/adapting paths of stuff as it propagates).

But then again, nothing like testing to be sure. It is also possible that https://github.com/arvidn/libtorrent/pull/6270 too would require some tweaks to handle this case correctly.

OP's use case seems legitimate, CMake projects (libraries, in particular) are supposed to be able to be used as subprojects in larger project hierarchies without issues. EDIT: @elelel You probably want to look into https://cmake.org/cmake/help/latest/module/FetchContent.html as well.

elelel commented 3 years ago

@arvidn I don't think that enforcing client's include directories from a dependency is the intention behind PUBLIC includes in CMake. In fact, the whole idea of a dependency library dictating the include path/third-party libs to it's parent in dependency node is not quite safe. Imagine there're two libraries being used by a project. And each of them decides which Boost to use with it's own methods. Now if both libraries push these paths/locations up the dependency graph how the conflict will be solved? It's entirely unpredictable and depends which one was the last to be added as add_subdirectory or in any other way. If a library intends to play nice with any tree-graph dependency structure, it either has to isolate it's subdependencies from the client, which is not possible in our case, because we need Boost .h interfaces, or it has to be configured from upstream (client) which must take the responsibility to find Boost and to guarantee that it's the same Boost for all it's dependencies.

PUBLIC keyword indicates that the include dirs, among other since, stay valid after you compiled the project (classic 'make build' equivalent) till you install the project ('make install'). A path within source tree is obviously not a valid path for "make install" stage. This is one of the few cases when it is clear that PUBLIC keyword is not used as supposed to be used by CMake developers, and they have the ability to catch that case and prevent compilation, which they do. If I place it to any other dir (not prefixed with source directory), there will be no problem compiling: CMake has no way to detect PUBLIC keyword usage which is still wrong. In general, I think it's out of libtorrent's scope of responsibility to manage client's third-party library dependencies.

FranciscoPombal commented 3 years ago

@elelel

I think you are misunderstanding how a large part of CMake is supposed to work. PUBLIC means a dependency is both used by our target and should be propagated as a dependency to targets that depend on our target (i.e., other projects). In CMake, you can think of PUBLIC as PRIVATE + INTERFACE. If libtorrent's CML is written correctly and the build systems of software that depends on libtorrent is also correctly configured (ideally they also use CMake), everything will be propagated automatically and the appropriate constraints will be calculated and resolved by CMake, and with the appropriate paths.

And yes, if libtorrent is built against a certain set of boost artifacts, it is expected that projects that link against libtorrent also use the same boost artifacts. What else did you expect? If I'm misunderstanding something here, please let me know.

As for build environment-dependent paths making their way into the dependency interface causing the build to fail because of that: that is obviously a problem, which as I speculated in https://github.com/arvidn/libtorrent/issues/6464#issuecomment-942223313, is almost for sure caused by using the ${Boost_INCLUDE_DIRS} variable to state the dependency on the header files instead of using a proper CMake target (target_link_libraries(torrent-rasterbar PUBLIC Boost::headers)) to do so (it introduces a hidden dependency on file paths, see https://gist.github.com/mbinna/c61dbb39bca0e4fb7d1f73b0d66a4fd1#dont-use-target_include_directories-with-a-path-outside-the-components-directory).

Again, I encourage you to try to reproduce the problem with https://github.com/arvidn/libtorrent/pull/6270.

See also the concept of BUILD_INTERFACE vs INSTALL_INTERFACE in CMake.

elelel commented 3 years ago

@FranciscoPombal

I think you are misunderstanding how a large part of CMake is supposed to work. PUBLIC means a dependency is both used by our target and should be propagated as a dependency to targets that depend on our target (i.e., other projects).

If you are using add_subdirectory style library dependencies PUBLIC keyword can't be used that way. Let's say I write a cool multinetwork p2p program. It uses libtorrent, and fictional libnapster and libedonkey. Each of them might want to use Boost. And the project on it's own also uses Boost. What makes libtorrent so special, that it has monopoly of finding Boost and pushing it upwards? Why libnapster can't do that? The only correct solution is to find boost in CMakeLists.txt of the project, and then pass the include/lib to the dependencies (to libtorrent, libnapster, libedonkey).

You mention INTERFACE and that is exactly the problem, libtorrent somehow thinks that a path obtained from FindBoost is a valid INTERFACE path. But it's not. It might be a precompiled Boost that is used only inside project and will never be published by the project in install target. When someone does "make install" with my p2p program I don't know that install scipt to know anything about my /home/user/project/source/lib/arm64/boost/include path. That is explained in more detail in links of this post: https://github.com/arvidn/libtorrent/issues/6464#issuecomment-929662247

I don't think I will be able to quickly reconfigure my cross-compilation project for #6270 to test it out any time soon, it requires rewriting quite a lot of CMake scripts. I'm totally okay with the hack I'm doing now, just patching the PUBLIC out of libtorrent's CMakeLists.txt

EDIT:

As for build environment-dependent paths making their way into the dependency interface causing the build to fail because of that: that is obviously a problem, which as I speculated in https://github.com/arvidn/libtorrent/issues/6464#issuecomment-942223313, is almost for sure caused by using the ${Boost_INCLUDE_DIRS} variable to state the dependency on the header files instead of using a proper CMake target ......

The reason of source path used as lib path for Boost is that because the correct Boost libraries are located there. They are located there because they are precompiled boost libraries and headers for cross-compilation for differrent architecture. When using FindBoost there are ways to hint it where to search the correct Boost. If you are using custom Boost, e.g. cross-compiling it will be some path like /project/lib/precompiled/arch/riscv/boost/include. It will be both in old-style variables as a result and in lib-interface targets. If such library happens to be located under same /src dir as the project's CMakeLists.txt, it is by definition non-public (can't be in install interface) and CMake detects that.

arvidn commented 3 years ago

What makes libtorrent so special, that it has monopoly of finding Boost and pushing it upwards? Why libnapster can't do that? The only correct solution is to find boost in CMakeLists.txt of the project, and then pass the include/lib to the dependencies (to libtorrent, libnapster, libedonkey).

It doesn't make one library special, all of them should (probably) propagate the boost dependency. If they disagree, the build system better fail. You can't have multiple different boost versions linked into the same binary. If the build system doesn't, that's a problem of the build system, not of the libraries you're linking against.

You mention INTERFACE and that is exactly the problem, libtorrent somehow thinks that a path obtained from FindBoost is a valid INTERFACE path. But it's not. It might be a precompiled Boost that is used only inside project and will never be published by the project in install target

That precompiled boost still needs matching headers for it, right? Using the correct headers is critical to not violate the ODR rule. It's fine if your target doesn't propagate the boost dependency by linking statically. But you can't have multiple boost versions in the same binary, that's a limitation of how programs are linked.

I would expect that if your binary links statically against boost, any install rule wouldn't care about installing any header nor libraries, since they're already statically linked against your binary. But I'm not familiar with how install targets work in cmake.

arvidn commented 3 years ago

that said, it does sound like there's an issue in the libtorrent cmakefile where it uses include directories explicitly at all, which it shouldn't be. but I don't see how the boost dependency itself can't be made public.

FranciscoPombal commented 3 years ago

@elelel

If you are using add_subdirectory style library dependencies PUBLIC keyword can't be used that way. Let's say I write a cool multinetwork p2p program. It uses libtorrent, and fictional libnapster and libedonkey. Each of them might want to use Boost. And the project on it's own also uses Boost. What makes libtorrent so special, that it has monopoly of finding Boost and pushing it upwards? Why libnapster can't do that? The only correct solution is to find boost in CMakeLists.txt of the project, and then pass the include/lib to the dependencies (to libtorrent, libnapster, libedonkey).

There are no such restrictions, which are leading you to a false "only correct solution".

It is perfectly fine for to have the following (assume you have a recent enough boost to satisfy all minimum requirements):

All projects will link to the same boost version, provided you provide the correct hints and your environment is correctly configured. libtorrent and libnapster will propagate the dependency on Boost to foo, while libedonkey will not (but it will use it internally). Furthermore, if foo uses Boost internally independently of these libraries, it should be explicit about it (add target_link_libraries(foo PRIVATE Boost::stuff)), because the libraries' interfaces may change to no longer include boost stuff, and compilation of foo will then break because it was only relying on the transitive link to Boost provided by at least one of the dependencies.

(the rest of the post)

I'm still convinced you should try out https://github.com/arvidn/libtorrent/pull/6270 and report your findings, it applies cleanly to RC_1_2 so it should not be difficult to checkout.

Overall you seem to be trying do rather complex stuff (cross compiling for different architectures and nesting projects), which I would love to be able to debug and investigate better. Unfortunately, I can't easily replicate your setup.

That being said, I would also advise you to experiment with:

Here is an excerpt of Professional CMake 10th edition (from the same author as in one of the sites you linked above) about system roots and how they are used in CMake:

In many cases, the toolchain is all that is needed, but sometimes projects may require access to a
broader set of libraries, header files, etc. as they would be found on the target platform. A common
way of handling this is to provide the build with a cut down version (or even a full version) of the
root filesystem for the target platform. This is referred to as a system root or just sysroot for short. A
sysroot is basically just the target platform’s root file system mounted or copied to a path that can
be accessed through the host’s file system. Toolchain packages often provide a minimal sysroot
containing various libraries, etc. needed for compiling and linking.

CMake has fairly extensive and easy to use support for sysroots. Toolchain files can set the
CMAKE_SYSROOT variable to the sysroot location and with that information alone, CMake can find
libraries, headers, etc. preferentially in the sysroot area over same-named files on the host (this is
covered in detail in Section 24.1.2, “Cross-compilation Controls”). In many cases, CMake will also
automatically add the necessary compiler/linker flags to the underlying tools to make them aware
of the sysroot area. For more complex scenarios where different sysroots need to be provided for
compiling and linking (e.g. as used by the Android NDK with unified headers), toolchain files can
set CMAKE_SYSROOT_COMPILE and CMAKE_SYSROOT_LINK instead when using CMake 3.9 or later.

In some arrangements, developers may choose to mount the full target file system under a host
mount point and use that as their sysroot. This could be mounted as read-only, or if not it may still
be desirable to leave it unmodified by the build. Therefore, when the project has been built, it may
need to be installed to somewhere else rather than writing to the sysroot area. CMake provides the
CMAKE_STAGING_PREFIX variable which can be used to set a staging point below which any install
commands will install to (see Section 26.1.2, “Base Install Location” for a discussion of this area).
This staging area could be a mount point for a running target system and the installed binaries
could then be tested immediately after installation. Such an arrangement is particularly useful
when cross compiling on a fast host for a target system that would otherwise be slow to build on
(e.g. building on a desktop machine for a Raspberry Pi target). Section 24.1.2, “Cross-compilation
Controls” also discusses how CMAKE_STAGING_PREFIX affects the way CMake searches for libraries,

Note: to better support cases where libtorrent is not the top level project, we can guard certain logic behind the following condition:

if(CMAKE_CURRENT_SOURCE_DIR STREQUAL CMAKE_SOURCE_DIR)
    # ...
endif()

The equality test can be replaced simply by one of these variables in CMake >= 3.21:

https://cmake.org/cmake/help/latest/variable/PROJECT_IS_TOP_LEVEL.html https://cmake.org/cmake/help/latest/variable/PROJECT-NAME_IS_TOP_LEVEL.html

elelel commented 3 years ago

@arvidn

That precompiled boost still needs matching headers for it, right? Using the correct headers is critical to not violate the ODR rule. It's fine if your target doesn't propagate the boost dependency by linking statically. But you can't have multiple boost versions in the same binary, that's a limitation of how programs are linked.

Correct, but that doesn't mean this very reasonable concern has to be addressed with a wrong instrument, that is, putting Boost headers into install interface. In case of add_subdirectory style I have only one single install artefect, in my client example, the statically compiled binary. This is the only file that will be installed, putting any header in it's install interface is not quite right.

But add_subdirectory scenario is not the main problem, the main problem is that you can't guarantee that the path you get from FindBoost is a path which is install-stable. Even for those people who do not use add_subdirectory but instead just compile the shared library and then install it, it is still not correct. Because you should not rely to any potentially temp path from source/build environment in your install script. Nobody runs into problem with this (yet), because not many people build the library against some temp-boost in their source dir, still it doesn't mean that what is happening correct.

@FranciscoPombal

It is perfectly fine for to have the following (assume you have a recent enough boost to satisfy all minimum requirements)

That will only work, if each of these library calls FindBoost and interprets it the same way. E.g. doesn't paramterize FindBoost by itself. And doesn't make wrong assumptions about it's path, for example. If you can give such guarantees, then I will trust the libraries and do it your way. Untill every library is 100% compliant in it's interpretation of FindBoost result and invokes it with same arguments, unfortunately, the only way for top project to be sure that Boost is found correctly and used correctly is to do the thing itself and pass the info down to dependencies.

That being said, I would also advise you to experiment with: Actually installing dependencies under some prefix (e.g. /projects/install) and then pointing CMAKE_PREFIX_PATH at it in projects that consume those dependencies. Since you mention that you already have precompiled libraries for different architectures for cross-compiling, you may want to treat the directory that all of those are under as your system root and point CMAKE_SYSROOT at it (inside a CMake toolchain file with the other necessary toolchain variables defined).

Perhaps I explained my situation badly. I have no problem with building my project, I perfectly understand why CMake complains. I also understand how to fix that in many ways. I prefer patching the PUBLIC into PRIVATE in my case because I don't want any dependency contaminate any other dependency or root project. Another solution, which is more robust and is probably more preferable by people who think that CMake developers are wrong and should be fooled out with their install interface path checks, is just to FetchContent from project src dir to any temp dir the precompiled Boost (copy). That way they'll never guess you're doing the nasty thing with install interface. CMAKE_PREFIX_PATH is completely not an option in case of my cross-compilation environment, and I can't imagine how it can be employed for this case in a reasonable way when cross-compiling either.

FranciscoPombal commented 3 years ago

@elelel

That will only work, if each of these library calls FindBoost and interprets it the same way. E.g. doesn't paramterize FindBoost by itself. And doesn't make wrong assumptions about it's path, for example. If you can give such guarantees, then I will trust the libraries and do it your way. Untill every library is 100% compliant in it's interpretation of FindBoost result and invokes it with same arguments, unfortunately, the only way for top project to be sure that Boost is found correctly and used correctly is to do the thing itself and pass the info down to dependencies.

Not sure what you mean by "doesn't paramterize FindBoost by itself". But there is no requirement that the dependency libraries of a project "invoke [FindBoost] with the same arguments". They just need to use it correctly and propagate the requirements correctly as exemplified above. If they are not doing that, then you should report the bug on their issue tracker. The only way for the top project to work correctly is for all of its dependencies to correctly propagate their usage requirements. As a top-level project, you don't "pass info down to dependencies", you consume their usage requirements. It is up to you to ensure the dependencies you are using were built (and possibly installed) with the correct options that your project requires, either manually, or with some source package manager-like solution such as vcpkg.

Nobody runs into problem with this (yet), because not many people build the library against some temp-boost in their source dir, still it doesn't mean that what is happening correct.

Another solution, which is more robust and is probably more preferable by people who think that CMake developers are wrong and should be fooled out with their install interface path checks, is just to FetchContent from project src dir to any temp dir the precompiled Boost (copy). That way they'll never guess you're doing the nasty thing with install interface. CMAKE_PREFIX_PATH is completely not an option in case of my cross-compilation environment, and I can't imagine how it can be employed for this case in a reasonable way when cross-compiling either.

not many people build the library against some temp-boost in their source dir,

Worth considering: maybe this isn't a "good" way of doing it in the first place? (regardless, and at the risk of sounding like a broken record, I still think the changes https://github.com/arvidn/libtorrent/pull/6270 can make the build robust against this)

Again, I don't know the details of your environment, but I'm inclined to agree that CMAKE_PREFIX_PATH won't solve your problem. At the same time, I'm more convinced (since you keep mentioning cross-compilation and precompiled boost binaries) that using CMAKE_SYSROOT (pointing somewhere outside of the source dir of the project you are building) is the solution you are looking for. FetchContent may also be of use like you say.

arvidn commented 2 years ago

I'm not sure what's actionable here. @elelel could you propose a patch in a PR?

There are examples that don't use boost, but when they link against libtorrent, the libtorrent headers they include will in turn include boost headers. Those headers will need to be found somewhere. Some dependency on boost need to propagate to the examples.

Perhaps I'm misunderstanding you and you're saying that this dependency should be propagated in the form of a public target dependency, not an include path dependency.

arvidn commented 2 years ago

If I make this change:

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 0d3432805..13e5ff4ca 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -793,7 +793,7 @@ endif()

 # Boost
 find_public_dependency(Boost REQUIRED)
-target_include_directories(torrent-rasterbar PUBLIC ${Boost_INCLUDE_DIRS})
+#target_include_directories(torrent-rasterbar PUBLIC ${Boost_INCLUDE_DIRS})
 if (Boost_MAJOR_VERSION LESS_EQUAL 1 AND Boost_MINOR_VERSION LESS 69)
        find_package(Boost REQUIRED COMPONENTS system)
        target_link_libraries(torrent-rasterbar PUBLIC ${Boost_SYSTEM_LIBRARY})

I get these build errors:

FAILED: CMakeFiles/torrent-rasterbar.dir/src/platform_util.cpp.o
/Library/Developer/CommandLineTools/usr/bin/c++ -DBOOST_ASIO_ENABLE_CANCELIO -DBOOST_ASIO_HAS_STD_CHRONO -DBOOST_ASIO_NO_DEPRECATED -DBOOST_EXCEPTION_DISABLE -DTORRENT_BUILDING_LIBRARY -DTORRENT_BUILDING_SHARED -DTORRENT_SSL_PEERS -DTORRENT_USE_GNUTLS -D_FILE_OFFSET_BITS=64 -Dtorrent_rasterbar_EXPORTS -Iinclude -Ideps/try_signal -Ideps/asio-gnutls/include -isystem /opt/homebrew/Cellar/gnutls/3.6.16_1/include -arch arm64 -isysroot /Library/Developer/CommandLineTools/SDKs/MacOSX11.3.sdk -fPIC -fvisibility=hidden -fvisibility-inlines-hidden -Weverything -Wno-c++98-compat-pedantic -Wno-c++11-compat-pedantic -Wno-padded -Wno-alloca -Wno-global-constructors -Wno-exit-time-destructors -Wno-weak-vtables -Wno-return-std-move-in-c++11 -Wno-unknown-warning-option -fexceptions -std=gnu++14 -MD -MT CMakeFiles/torrent-rasterbar.dir/src/platform_util.cpp.o -MF CMakeFiles/torrent-rasterbar.dir/src/platform_util.cpp.o.d -o CMakeFiles/torrent-rasterbar.dir/src/platform_util.cpp.o -c src/platform_util.cpp
In file included from src/platform_util.cpp:35:
include/libtorrent/config.hpp:49:10: fatal error: 'boost/config.hpp' file not found
#include <boost/config.hpp>
         ^~~~~~~~~~~~~~~~~~
1 error generated.

I suppose there's supposed to be an add_dependencies(torrent-rasterbar, libtorrent-rasterbar) somewhere

elelel commented 2 years ago

If I make this change...

I assume you are compiling only CMakeLists.txt from libtorrent and not a client's CMakeLists.txt in this example. If that is the case, the patch that is correct would be just:

-target_include_directories(torrent-rasterbar PUBLIC ${Boost_INCLUDE_DIRS})
+target_include_directories(torrent-rasterbar PRIVATE ${Boost_INCLUDE_DIRS})

This compiles without the errors:

...
[100%] Linking CXX shared library libtorrent-rasterbar.so
[100%] Built target torrent-rasterbar

But that of course means, that a client that uses such a lib's CMakeLists.txt both indirectly (when using later installed libtorrent as a system-found lib, for example) or directly (by using add_subdirectory(...) and pointing to libtorrent's source dir in it's own CMakeList.txt) now has to make target_include_directories(.... Boost) on it's own. If I understand correctly why a "PUBLIC" include is made now, it is such as to free the client of the responsibility to make target_include_directory(... Boost) statement in it's file on it's own. Again, if I understand correctly the reason for that is to make sure that the client's CMakeLists and libtorrent's CMakeLists include the same thing (Boost headers at the same path). But unfortunately it's not what "PUBLIC" keyword is for. "PUBLIC" keyword adds the headers listed in a CMakeLists.txt to CMake's install interface, for the whole project, not just for libtorrent. That means if a client has a CMakeLists.txt that uses libtorrent's source files by making a call to add_subdirectory to libtorrent's source directory, get's these paths into it's install interface. But when you create an end-user application the only public artefact is your final executable and that is the thing that will be "make install"-ed. It's exactly that reason why CMake authors prevented compiling anything they can detect as a non-public location, in my concrete example, source dir location of a public header. It happens that now nobody places Boost headers into their project's src dir, because it's rarely needed, and they are not hitting this error. But it's still ok to do that, there is no reason to say "never place your precompiled Boost libs/headers to your project's source dir". So in terms of actionability I see three options:

  1. Just say explicitly that the client should not add libtorrent's source directory directly by making something like add_subdirectory(path_to_libtorrent libtorrent), because that is not supported and the lib's CMakeLists.txt should be used only for making and installing standalone libtorrent library. Explain in docs that it is not supported because libtorrent's CMakeLists.txt puts Boost header paths in whatever global (higher-level) CMakeLists target's install interface, but it has no way of knowing if these paths may be propagated up the dependency chain as paths for install interface at all.
  2. Make target_include_directories(.... PRIVATE ${Boost_INCLUDE_DIRS}) and document that the client's CMakeLists.txt has to make target_include_directories(.... PRIVATE/PUBLIC ${Boost_INCLUDE_DIRS}) statement for it's includes on it's own. In that case add_subdirectory dependency style will be supported.
  3. Make something like a CMake option to branch between PUBLIC/PRIVATE includes depending how the client wants to use it. Note that this is still not formally a correct solution, because ${Boost_INCLUDE_DIRS} contains just header location where Boost was found, not header locations that are valid to be put into top target's insall interface.
arvidn commented 2 years ago

I assume you are compiling only CMakeLists.txt from libtorrent and not a client's CMakeLists.txt in this example. If that is the case, the patch that is correct would be just:

Right, you suggested keeping it, but making it private.

As you point out, this change still makes users/clients of the library to fail to build, for example:

[1/411] Building CXX object examples/CMakeFiles/stats_counters.dir/stats_counters.cpp.o
FAILED: examples/CMakeFiles/stats_counters.dir/stats_counters.cpp.o
/Library/Developer/CommandLineTools/usr/bin/c++ -DBOOST_ASIO_ENABLE_CANCELIO -DBOOST_ASIO_NO_DEPRECATED -DTORRENT_EXPORT_EXTRA -DTORRENT_LINKING_SHARED -DTORRENT_SSL_PEERS -DTORRENT_USE_GNUTLS -Iinclude -Ideps/asio-gnutls/include -isystem
 /opt/homebrew/Cellar/gnutls/3.6.16_1/include -arch arm64 -isysroot /Library/Developer/CommandLineTools/SDKs/MacOSX11.3.sdk -Weverything -Wno-c++98-compat-pedantic -Wno-c++11-compat-pedantic -Wno-padded -Wno-alloca -Wno-global-constructor
s -Wno-exit-time-destructors -Wno-weak-vtables -Wno-return-std-move-in-c++11 -Wno-unknown-warning-option -Wno-implicit-int-float-conversion -fexceptions -std=gnu++14 -MD -MT examples/CMakeFiles/stats_counters.dir/stats_counters.cpp.o -MF 
examples/CMakeFiles/stats_counters.dir/stats_counters.cpp.o.d -o examples/CMakeFiles/stats_counters.dir/stats_counters.cpp.o -c examples/stats_counters.cpp
In file included from examples/stats_counters.cpp:33:
In file included from include/libtorrent/session_stats.hpp:36:
include/libtorrent/config.hpp:49:10: fatal error: 'boost/config.hpp' file not found
#include <boost/config.hpp>
         ^~~~~~~~~~~~~~~~~~
1 error generated.

If I understand correctly why a "PUBLIC" include is made now, it is such as to free the client of the responsibility to make target_include_directory(... Boost) statement in it's file on it's own. Again, if I understand correctly the reason for that is to make sure that the client's CMakeLists and libtorrent's CMakeLists include the same thing (Boost headers at the same path).

I wouldn't quite phrase it that way. It is the build systems responsibility to propagate dependencies required by the client. If libtorrent adds a dependency on some other library, libtorrent's build files should change, not all users'.

From your description, it sounds like cmake does not support this. Which would surprise me because it seems like one of the most important features of a build system (and the core of "modern" cmake).

How come this doesn't work?

--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -793,10 +793,10 @@ endif()

 # Boost
 find_public_dependency(Boost REQUIRED)
-target_include_directories(torrent-rasterbar PUBLIC ${Boost_INCLUDE_DIRS})
+target_link_libraries(torrent-rasterbar PUBLIC Boost)
 if (Boost_MAJOR_VERSION LESS_EQUAL 1 AND Boost_MINOR_VERSION LESS 69)
        find_package(Boost REQUIRED COMPONENTS system)
-       target_link_libraries(torrent-rasterbar PUBLIC ${Boost_SYSTEM_LIBRARY})
+       target_link_libraries(torrent-rasterbar PUBLIC Boost::system)
 endif()

 if (exceptions)

the idea being that raising the abstraction level to "targets" rather than "include paths" would allow cmake to be smarter about it. With this patch, the examples still fail to include boost.

arvidn commented 2 years ago

this looks promising. I need to experiment a bit:

--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -792,11 +792,11 @@ else()
 endif()

 # Boost
-find_public_dependency(Boost REQUIRED)
-target_include_directories(torrent-rasterbar PUBLIC ${Boost_INCLUDE_DIRS})
+find_public_dependency(Boost REQUIRED COMPONENTS headers)
+target_link_libraries(torrent-rasterbar PUBLIC Boost::headers)
 if (Boost_MAJOR_VERSION LESS_EQUAL 1 AND Boost_MINOR_VERSION LESS 69)
        find_package(Boost REQUIRED COMPONENTS system)
-       target_link_libraries(torrent-rasterbar PUBLIC ${Boost_SYSTEM_LIBRARY})
+       target_link_libraries(torrent-rasterbar PUBLIC Boost::system)
 endif()

 if (exceptions)
elelel commented 2 years ago

As you point out, this change still makes users/clients of the library to fail to build, for example: [1/411] Building CXX object examples/CMakeFiles/stats_counters.dir/stats_counters.cpp.o

The above point about building I made are relevant to libtorrent's CMakeLists, not to the clients that use it (e.g. example's CMakeLists. To get them compiled each of them has to include in their CMakeLists line like:

find_public_dependency(Boost REQUIRED)
target_include_directories(stats_counters PUBLIC ${Boost_INCLUDE_DIRS})

And this makes sense from pure C perspective, after all that #include that build fails on fails when compiling a example's translation unit because it's -I parameter doesn't include Boost's dir. It's not that it libtorrent's build itself fails.

From your description, it sounds like cmake does not support this. Which would surprise me because it seems like one of the most important features of a build system (and the core of "modern" cmake).

I wouldn't say that it's CMake who doesn't support this, it's C compile system that does not quite support it. CMake is just a set of convinience wrappers that emulate a "build system" analogous to modern languages on top of standard C approach of specifying dependencies that comes from 1970s. And that approach comes down to two things basically: compilier invocation accepts a plain list of headers that are combined in single plain and large header source and libraries for dynamic linking passed in through linker invocation. C compiler does nothing about "structure" which is unlike other languages like Java. What it "sees" is a single .h source pseudofile that is glued together from pieces of #include that come from actual .h files. CMake tries to do it's best to provide an -I directective that would produce such a pseudofile that makes sense. It may look as if it is fully analogous to mdoern package management for CMake users, but it is still just an emulation which is limited by underlying C build system. Because of such limitations they prevent people from doing certain otherwise natural in other build systems things.

Purely from graph-theoretic view, to take a directed tree of library dependencies and produce an undirected chain graph that represents ordered parameters for cc -I argument, one needs deterministic rules of resolving conflicts between the nodes (e.g. when they include the same thing but of differrent versions) and general consistency guarantees (e.g. we depend on header locations that may change AFTER the compilation has been done, for example system-wide install-time h dependencies for a library; Java doesn't have this problem for example). CMake is just trying to act within the scope where more or less formal rules hold as much as possible.

this looks promising. I need to experiment a bit:

-target_include_directories(torrent-rasterbar PUBLIC ${Boost_INCLUDE_DIRS})
+find_public_dependency(Boost REQUIRED COMPONENTS headers)
+target_link_libraries(torrent-rasterbar PUBLIC Boost::headers)

I'm not sure, but it doesn't look to be correct. target_link_libraries wouldn't add anything to includes, maybe only in some special platform-dependent cases?

elelel commented 2 years ago

Maybe a better perspective to understand the issue is just to look at source of CMake's FindBoost: https://github.com/Kitware/CMake/blob/master/Modules/FindBoost.cmake

What it basically does, it searches the system on which that script is invoked for boost files of specific versions, taking into considiration various user-defined hint variables. So the only thing it can produce is paths to files that are valid only at build time. It's like running something like "find / | grep boost" on the system where we are building. The PUBLIC keyword in target_include_directories puts the arguments into CMake target install interface. This means that these files/paths are available and valid not only at build time, but also at install time, the time when user types "make install". That is what they mean by keeping the paths in the build "portable" in StackOverflow links I mentioned above. We can't be sure that a paths that we got by browsing build computer's filesystem is valid for every system where we run make install.

arvidn commented 2 years ago

I'm not sure, but it doesn't look to be correct. target_link_libraries wouldn't add anything to includes, maybe only in some special platform-dependent cases?

This is how "modern" cmake works (and also other proper build systems).

The documentation is here:

Specify libraries or flags to use when linking a given target and/or its dependents. Usage requirements from linked library targets will be propagated. Usage requirements of a target's dependencies affect compilation of its own sources.

arvidn commented 2 years ago

@elelel does this patch fix your issue? https://github.com/arvidn/libtorrent/pull/6570 Is the problem that an invalid path to boost is printed in the package config file? If not, how is the invalid boost header path propagated to your build?

elelel commented 2 years ago

From the same page:

Note that it is not advisable to populate the INTERFACE_LINK_LIBRARIES of a target with absolute paths to dependencies. That would hard-code into installed packages the library file paths for dependencies as found on the machine the package was made on.

I don't know if the same check that prevents putting fishy paths in install interface exists to prevent fishy paths in link interface. It should. It can also be possible that target_link_libraries(... PUBLIC) make target_include_directories(... PRIVATE), so it will be equivalent to just taking option 2 of what I suggested, and in this case it should work. The patch does not work for me though: it compiles x86_64 on x86_64 host, but does not compile arm7 on x86_64 host. Apparently it does not catch my configuration hint for cross-compilation and succeeds with host x86_64 headers. I will reconfigure the project and check in more details later. But frankly I don't see the point changing the way you put non-public path into cmake target's public interface. It is still a path that should not be there, whether you trick CMake into accepting it or not.

By the way, the fact that you put something as PUBLIC or PRIVATE does not affect your library's client unless it includes your CMakeLists into their CMakeLists which most people don't do. For most people on Unix-like systems Boost headers are found by their project because they are at the same location as libtorrent's headers, somehere down /usr/include.

elelel commented 2 years ago

I think I came up with a way that both should work with your approach and with cross-compilation structure when part of brebuilt target arch system resides within project src. In client CMakeLists.txt must do something like this:

# Get the path of additional "system root" for all target arches.
# In this case it is 3 directories higher than the location of CMakeLists.txt
get_filename_component(FRP ${CMAKE_CURRENT_SOURCE_DIR} PATH)
get_filename_component(FRP ${FRP} PATH)
get_filename_component(FRP ${FRP} PATH)
# Add that group "root" as additional root path for cmake_find
list(APPEND CMAKE_FIND_ROOT_PATH ${FRP} )
# Set arch-specific locations for headers and libraries
# In this case it is based on standard Android CMake cross-compile vars.
set (Boost_ROOT "/")
set (BOOST_LIBRARYDIR "/libs.built/boost/${CMAKE_ANDROID_ARCH_ABI}" cache string "" force)
set (BOOST_INCLUDEDIR "/include.built/boost/${CMAKE_ANDROID_ARCH_ABI}" cache string "" force)
# Call FindBoost with all the libs that libtorrent might later want, as well as with the libs your program requires itself
find_package(Boost REQUIRED system)
# Include libtorrent's source.
add_subdirectory("external/git/libtorrent" libtorrent)

Since the state of Boost::headers is cached, when libtorrent calls find_package(...) again it will just get the right result. This works and I think it is the clean way to go. FindBoost.cmake adds the Boost header as INTERFACE header, not PUBLIC: https://github.com/Kitware/CMake/blob/56e6f68d5a62168f3ac70e80f7d7e69653ccb826/Modules/FindBoost.cmake#L547 I don't know how it is handled later if target_link_libaries is called with PUBLIC keyword. My guess when you add it like this the headers dir won't get into install public interfaces. Or maybe adding another system root to the list just prevents the check to be triggered. At the same time this approach seems to work also with the old way (target_include_directories PUBLIC).

For reference, I found previous discussion of the issue #3101

stale[bot] commented 2 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.