On 12. 6. 25 22:36, Timofei Zhakov wrote:
On Thu, Jun 12, 2025 at 7:51 PM Branko Čibej <br...@apache.org> wrote:
On 12. 6. 25 19:10, Daniel Sahlberg wrote:
Den tors 12 juni 2025 kl 00:34 skrev Timofei Zhakov
<t...@chemodax.net>:
Revert most of the changes in svn_utf.h from r1926293.
Instead, tell the
CMake build to expose APR and APR-Util include paths
everywhere.
(...)
* CMakeLists.txt: Add APR and APR-Util include paths to
the top-level
directory properties.
(...)
--- subversion/trunk/CMakeLists.txt (original)
+++ subversion/trunk/CMakeLists.txt Wed Jun 11 15:40:43 2025
@@ -289,6 +289,12 @@ else()
endif()
endif()
+# APR and APR-Util include directories must be available
to all our souroces,
+# not just those that happen to link with one or the
other of these libraries.
+get_target_property(_apr_include external-apr
INTERFACE_INCLUDE_DIRECTORIES)
+get_target_property(_apu_include external-aprutil
INTERFACE_INCLUDE_DIRECTORIES)
+include_directories(${_apr_include} ${_apu_include})
+
This is wrong. You can't modify global include directories of
the entire project to force it to use apr everywhere. This is
a hack, not a fix.
As far as I understand CMake would add the include directories
automatically on targets that link APR (or APR-U... havn't
wrapped my head around what goes where yet), but the issue is
that it doesn't do that when not linking the libraries - is that
correct?
CMake has something called "imported targets", i.e., libraries
that the project uses but doesn't build. CMake targets have
various properties, one of them (the above
INTERFACE_INCLUDE_DIRECTORIES) says, basically, "code that uses
this library will find the needed headers here"). That's
simplified, but for C and C++, it's what the property boils down to.
These include paths are only injected into the compiler options
when a target is a dependency of the source that's being compiled.
In our case, what that means is that if one of subversion's
libraries or tools directly links apr-util, for example, the
apr-util include path will be added to CFLAGS (or rather,
C_INCLUDES in cmake's world) when the sources of that library are
compiled.
If a source doesn't link with apr-util but /does/ include one of
the apr-util headers, then we have a problem. It wasn't noticed
before because all the GH builders, and probably all the current
developers' setups, have APR and APR-Util installed in the same
place; and since all of our artefacts link with libapr, the
apr-util headers were always available -- even though the the
build script doesn't take that into account.
Homebrew on the Mac installs APR and APR-Util in different
directories, so this issue showed up when I tried to use the CMake
build on a mac (in order to test the patch to FindSerf.cmake with
serf-2).
The proximal reason being that some libraries that don't link with
apr_util do use the svn_utf.h header, which includes apr_xlate.h
from apr-util, which wasn't available, which caused a 💥. Those
libraries and tools only use apr-util indirectly, thorough
libsvn_subr.
Three possible solutions came to mind:
* Change build.conf, where the library dependencies are defined,
to add the dependency on apr-util. This would cause those
targets to link libaprutil, even when they're not using any
symbols from it. Thad didn't seem right.
* Change svn_utf.h to avoid that dependency ... which we know we
can't, due to backward-compatibility concerns.
* Fix the include paths in CMake.
That last part has several possible solutions. I implemented the
most obvious one, i.e., adding APR and APR-Util include paths to
all Subversion's build targets. It's an obvious solution because
APR and APR-Util are essentially part of Subversion's public
interface (because of our use of pools in the API, and APR's
integer types, and containers, etc.).
[An aside: if we decided to export our CMake targets to make them
directly usable by downstream, those apr-util include paths will
have to be included in our targets' INTERFACE_INCLUDE_DIRECTORIES.
This is essentially what we do in the pkg-config files, too, which
are the equivalent of cmake's exported targets.)
A better way would be to convince CMake to take the whole
transitive dependency tree into account, but, after having spent
way too much time chasing down CMake (3.12) documentation, I admit
that I don't know how to do that.
What is the CMake-ish solution here?
Since we generate the CMake target code from build.conf - can we
add a new parameter that add these include directories only on
the targets that actually require them?
Before someone jumps head-first in to the spaghetti that is our
build.conf generator, answer this question: why should we optimize
the include paths to the absolute minimum required, when that is
clearly a problem now and can't be anything but a problem down the
road? I'm talking specifically about APR and APR-Util include
paths, which merge into one in apr-2 in any case.
-- Brane
Thanks for your explanation!
I'd add a few points from myself.
When a library is linked into another target through
target_link_libraries(), it can be either PUBLIC, PRIVATE, or INTERFACE:
- PRIVATE means that the dependency will be entirely used in the
following target (both include directories and link flags are copied
to the new target). For example, when APR is linked into
libsvn_something as a PRIVATE dependency, this library gains access to
APR functions and headers. However, it doesn't spread to the target
depending on our libsvn_something (like our main svn.exe). Actually,
there is an exception that copies link flags in a static build, but
it's a different story. I mean it's not the main point.
As for another, more real world example, libsvn_fs_fs is a completely
internal library without any public APIs. I mean those used in our
programs. They usually only link against let's say libsvn_fs (in the
most abstract scenario). libsvn_fs does link libsvn_fs_fs, but doesn't
use it in its public api. This all means that if it's linked as a
PRIVATE dependency, no includes of libsvn_fs_fs will be available for
libsvn_fs. (let's imagine they would be in different directories for
clarity)
Also, answering Branko's question, optimising include directories
might be helpful to explicitly control whether the dependency is used
or not. For example, our libsvn_diff doesn't use apr util. So if
someone would call apr util api on accident, we will notice this
because the build would no longer be passing.
Yup. And libsvn_diff doesn't link apr-util, build.conf says so and all
our generated target dependencies derive from that. However:
trunk/subversion/libsvn_diff$ grep svn_utf.h *.c
deprecated.c:#include "svn_utf.h"
diff_file.c:#include "svn_utf.h"
diff_memory.c:#include "svn_utf.h"
parse-diff.c:#include "svn_utf.h"
util.c:#include "svn_utf.h"
So it needs -I/some/path/to/apr-util/include/apr-1 but it doesn't need
libaprutil.so (or .a or .lib or .dylib etc.). This is exactly why I
didn't want to change the dependency lists in build.conf.
- INTERFACE linkage only affects the targets dependent on the library.
It does literally nothing on the target itself, but spreads onto
dependent libraries.
- PUBLIC dependencies are a sort of combination of those both types.
For example, apr is the best example of a PUBLIC dependency, because
it is used in the library itself and required in the public header
files (for example libsvn_client exports svn_client.h, which includes
apr.h. Meaning that apr.h is a part of libsvn_client public api).
Right now libsvn_subr has the following dependencies:
target_link_libraries(libsvn_subr PRIVATE
external-aprutil
external-apr
external-xml
external-zlib
external-sqlite
external-intl
external-lz4
external-utf8proc
)
I think it should be like:
target_link_libraries(libsvn_subr PUBLIC
external-aprutil
external-apr
)
target_link_libraries(libsvn_subr PRIVATE
external-xml
external-zlib
external-sqlite
external-intl
external-lz4
external-utf8proc
)
Initially, I thought that all dependencies are explicitly listed in
the build.conf, so I didn't take the effort that time. Should I rework
the generator to produce such targets?
The link dependencies are in build.conf. The public/private distinction
isn't (since that doesn't affect linking a target). The autotools build,
based on build.conf, and also the generated Visual Studio files, make
all include paths available to all targets. I'd have to double-check,
but I think the exceptions to this rule are the (Swig) bindings, which
get some extra help.
This would be the cmake-ish way to fix this problem. And just an
improvement.
Well, I'd hate to add an explicit flag in build.conf just to distinguish
between public and private dependencies. Having said that, we only have
issues with external-aprutil (and, to some degree, external-apr, since
they should be treated in the same way). So how about adding this
knowledge directly into build/generator/gen_cmake.py? It is, after all,
its job is to generate correct dependencies for cmake. It already adds
the "external-" prefix to some of the library names; it may as well put
apr and apr-util into the PUBLIC space.
The attached patch appears to work for me.
-- Brane
Index: CMakeLists.txt
===================================================================
--- CMakeLists.txt (revision 1926360)
+++ CMakeLists.txt (working copy)
@@ -289,12 +289,6 @@
endif()
endif()
-# APR and APR-Util include directories must be available to all our souroces,
-# not just those that happen to link with one or the other of these libraries.
-get_target_property(_apr_include external-apr INTERFACE_INCLUDE_DIRECTORIES)
-get_target_property(_apu_include external-aprutil
INTERFACE_INCLUDE_DIRECTORIES)
-include_directories(${_apr_include} ${_apu_include})
-
### ZLIB
if(SVN_USE_PKG_CONFIG)
Index: build/generator/gen_cmake.py
===================================================================
--- build/generator/gen_cmake.py (revision 1926359)
+++ build/generator/gen_cmake.py (working copy)
@@ -53,6 +53,10 @@
return name[7:].upper()
+# APR and APR-Util are part of our public interface and should be
+# declared PUBLIC in library target dependencies.
+PUBLIC_LIB_DEPENDS = frozenset(["external-apr", "external-aprutil"])
+
def get_output_name(target):
if target.name.startswith("lib"):
return target.name[3:] + "-1"
@@ -146,7 +150,8 @@
msvc_export.append("subversion/include/" + export)
sources = []
- libs = []
+ private_libs = []
+ public_libs = []
for dep in self.get_dependencies(target.name):
enable_condition += get_target_conditions(dep)
@@ -157,9 +162,9 @@
elif isinstance(dep, gen_base.TargetLinked):
if dep.external_lib:
if dep.name == "ra-libs":
- libs.append("ra-libs")
+ private_libs.append("ra-libs")
elif dep.name == "fs-libs":
- libs.append("fs-libs")
+ private_libs.append("fs-libs")
elif dep.name in ["apriconv",
"apr_memcache",
"magic",
@@ -170,9 +175,14 @@
# TODO:
pass
else:
- libs.append("external-" + dep.name)
+ dep_name = "external-" + dep.name
+ if (dep_name in PUBLIC_LIB_DEPENDS
+ and not isinstance(target, gen_base.TargetExe)):
+ public_libs.append(dep_name)
+ else:
+ private_libs.append(dep_name)
else:
- libs.append(dep.name)
+ private_libs.append(dep.name)
elif isinstance(dep, gen_base.ObjectFile):
for source in self.graph.get_sources(gen_base.DT_OBJECT, dep,
gen_base.SourceFile):
@@ -212,7 +222,9 @@
output_name = get_output_name(target),
type = target_type,
sources = sources,
- libs = libs,
+ libs = public_libs + private_libs,
+ public_libs = public_libs,
+ private_libs = private_libs,
msvc_libs = msvc_libs,
msvc_objects = msvc_objects,
msvc_export = msvc_export,
Index: build/generator/templates/targets.cmake.ezt
===================================================================
--- build/generator/templates/targets.cmake.ezt (revision 1926359)
+++ build/generator/templates/targets.cmake.ezt (working copy)
@@ -69,9 +69,12 @@
WORKING_DIRECTORY $<TARGET_FILE_DIR:[targets.name]>
)
set_tests_properties([targets.namespace].[targets.name] PROPERTIES
ENVIRONMENT LD_LIBRARY_PATH=$<TARGET_FILE_DIR:[targets.name]>)
- [end]target_link_libraries([targets.name] PRIVATE[for targets.libs]
- [targets.libs][end]
- )[if-any targets.msvc_libs]
+ [end][if-any targets.public_libs]target_link_libraries([targets.name]
PUBLIC[for targets.public_libs]
+ [targets.public_libs][end]
+ )
+ [end][if-any targets.private_libs]target_link_libraries([targets.name]
PRIVATE[for targets.private_libs]
+ [targets.private_libs][end]
+ )[end][if-any targets.msvc_libs]
if (WIN32)
target_link_libraries([targets.name] PRIVATE[for targets.msvc_libs]
[targets.msvc_libs][end])
endif()[end][if-any targets.msvc_objects]
