Den fre 13 juni 2025 kl 02:35 skrev Branko Čibej <br...@apache.org>:
> On 13. 6. 25 01:14, Timofei Zhakov wrote:
>
> (...cut...)
>
>
>> 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.
>>
>
> +1, except for several minor notices.
>
> 1. What is the actual purpose of a frozenset PUBLIC_LIB_DEPENDS? I mean
> just above we are performing exactly the same check, but using an '[...]'
> construction and I think it's not worthed to optimise performance of a
> build script and we use it just once. So why not do this instead?:
>
>
>
> Eh, that's just me, optimizing too soon. A linear lookup would be as fast
> as a hash lookup for two elements. Plus, there's a nice place at the top of
> the script for a comment.
>
>
> @@ -170,9 +175,14 @@
> # TODO:
> pass
> else:
> - libs.append("external-" + dep.name)
> + dep_name = "external-" + dep.name
> + if (dep_name in ["external-apr", "external-aprutil"]
> + 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):
> 2. It's maybe just an enhancement for the future, if we decide to, for
> example, export a cmake config, but it might be better to treat some libsvn
> dependencies as PUBLIC. For example, I'm sure most of our headers include
> something from libsvn_subr, so technically it's a public dependency for
> them. This most probably would not change anything since they have the same
> include directory, but still, in theory it could.
>
>
> In that case, we could just add these to the existing list of
> always-public dependencies, right?
>
> Anyway, go ahead and commit the patch with or without any changes.
>
> -- Brane
>
Thanks guys for the excellent explanations in the last few emails and nice
cooperation on this patch. If I understand it correctly, the issue would be
resolved by making this a PUBLIC dependency.
I'm in general in favour of making as much as possible a configuration and
having the code just understanding configuration. But I'm fine hardcoding
stuff as well in the CMake-focused parts of gen-make. In that case it is
probably easier to have the list (set?) of special-cased dependencies as
close to where they are used as possible.
So I'm +1 (concept - I'm not enough of an expert to fully review but I
think the explanations sounds reasonable) - please go ahead and commit.
Kind regards,
Daniel
