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 <http://dep.name>)
+ dep_name = "external-" + dep.name <http://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 <http://dep.name>)
+ private_libs.append(dep.name <http://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
