(...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?:
@@ -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.
--
Timofei Zhakov
