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.
- 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?
This would be the cmake-ish way to fix this problem. And just an
improvement.
--
Timofei Zhakov
