On Mon, Jun 2, 2025 at 7:04 AM Branko Čibej <br...@apache.org> wrote:
> On 2. 6. 25 06:33, Branko Čibej wrote:
> > On 1. 6. 25 23:40, Timofei Zhakov wrote:
> >> On Fri, 30 May 2025 at 4:44 PM, Branko Čibej <br...@apache.org> wrote:
> >>
> >> Hi!
> >>
> >> With the latest bunch of commits, I'm tentatively hopeful that the
> >> CMake
> >> build is now on par with the SCons build. But it needs a lot of
> >> testing
> >> and review.
> >>
> >> One part where I'm a bit baffled is generating the pkg-config
> >> file. The
> >> more I look at it, the more it seems like the library list is
> >> either too
> >> long or incomplete, and the structure is inconsistent. It'd save
> >> me a
> >> lot of grief if someone more knowledgeable could have a look at
> >> that...
> >>
> >> I'm also unsure if we should generate a serf-config.cmake for
> >> downstream
> >> consumers, and where to install it if we do. CMake docs are less
> >> than
> >> specific, bit something like $prefix/lib/cmake/ or
> >> $prefix/share/cmake
> >> seems to be right for Unix, and just $preifx/cmake for Windows.
> >>
> >> Anyway. I tested this on two platforms:
> >>
> >> * arm64 macOS, with both Homebrew and MacPorts dependencies:
> >> o with Homebrew, I used the default system zlib
> >> o there is a system GSSAPI but if I use it, the tests go
> >> into an
> >> infinite loop
> >> * arm64 Windows, using MSBuild (with Visual Studio) and vcpkg for
> >> dependencies. Nothing very special there except that the
> >> Windows VM
> >> is horribly slow.
> >>
> >> I didn't test with APR trunk (apr-2), perhaps I should. It's at
> >> least
> >> theoretically supported, and I'm sure it worked at one time, but
> >> I'll
> >> have to build that first.
> >>
> >> So, any and all feedback welcome.
> >>
> >> -- Brane
> >>
> >> P.S.: No, I am *not* a CMake maestro. I keep a browser window with
> >> cmake-3.12 docs open and that's pretty much it. I'm sure some things
> >> could be done more modernly CMake-ish.
> >>
> >>
> >> I remember when I tried cmake on my own, I didn’t really enjoy the
> >> experience. It was working properly in general, but the thing is it
> >> wasn’t felt finished. (Again, I’m talking about its state in the
> >> past. I didn’t try it after your fixes.)
> >>
> >> In my honest opinion, cmake scripts must contains as little custom
> >> code as possible. I mean it should be written as intending by design,
> >> keeping proper structure, and making it useful for users.
> >
> > Sure I agree with all that.
> >
> >> Basically, as the most complicated part in the build system is
> >> dependency search, they designed some sort of framework for that.
> >> They made it so you can write a module in like a few commands. This
> >> is how I made modules for apr and other libs in subversion. I’d like
> >> to suggest (/ agree with you!) that we could merge those. In a recent
> >> email to dev@s.a.o, you have mentioned that we should finish serf’s
> >> one and use it in subversion. I would argue with that, because they
> >> are done in a wrong way from what was intended.
> >
> >
> > Well I dunno, It pretty much works in almost any circumstances by now,
> > no matter what was "intended". Remember that this effort started
> > before there was a CMake 3, let alone 4; it was all 2.something, which
> > lacked a _lot_ of what you mention below. Rewriting the whole build
> > configuration every time the CMake guys think up a new way of doing
> > things, out with the old!, is really not something I'd like to spend
> > time on.
> >
> > It's bad enough that SCons has backward-incompatible changes. Give me
> > autotools (without automake) any day, and also give me a big stick to
> > use on the monster who decided that it should all be written in m4 and
> > Bourne shell...
> >
> >
> >> I guess, in subversion, we are missing few functionalities that serf
> >> wants (pkgconfig search, apr-2 support, and emmm that’s it?). I think
> >> we can simply implement those features in subversion and use the same
> >> files in serf.
> >
> > And stuff that Subversion doesn't use, like Brotli, OpenSSL, GSSAPI
> > (on not-Windows). Of those three, only OpenSSL ships a standar
> > Find*.cmake script with CMake.
> >
> >> On the other hand, serf is 1) incorrectly creates targets,
> >
> > Please explain some more.
> >
> >> 2) wants the root path to be passed manually, 3) weirdly executes
> >> aprconfig (there is actually cmake module for that, which does it
> >> automatically.
> >
> > A CMake module for running apr-{1,2}-config and apu-1-config? Wow.
> >
> >> I have no idea why this wasn’t chosen initially), and 4) is little
> >> messy.
> >
> > The root dirs are required on Windows, not otherwise. If you use
> > dependencies from vcpkg, you don't need that -- but that's a vcpkg
> > thing, APR's CMake build is still experimental; although I see it
> > generates an apr-cmake-version.cmake file, at least on trunk. This is
> > relatively recent, older APR versions (that we still support) don't
> > have that. And anyway, that file is not installed on, e.g., Debian 12
> > with libapr1-dev (which is apr-1.7.2, so relatively recent).
> >
> >> Oh, and actually you can’t currently build serf without specifying
> >> extra parameters for apr dir, which is quite confusing.
> >
> > Which extra parameters?
> >
> >
> >> That’s it… sorry if I’m wrong in any point. I’m open to discussing.
> >
> > I'm happy to see any improvement in the CMake build, as long as it
> > still works and has all the features we need. :)
> >
> > My goal was to make it work on pretty much all platforms in all
> > conceivable weird layouts of the modules, I didn't care too much about
> > "user-friendly". It's a build tool, not a phone app.
>
> By the way, on Linux I just have to run: "cmake <source dir>", and it
> just works, finding all the installed dependencies.
>
> On macOS it's /almost/ there; which MacPorts and /opt/local/bin in the
> PATH, it finds everything except the MacPorts zlib -- it takes the
> system version, which is not so good because other MacPorts dependencies
> use a different zlib. And since I have both Homebrew and MacPorts at the
> moment, it picks some packages from each, pretty much guaranteeing
> disaster. That's why I added USE_HOMEBREW and USE_MACPORTS options.
>
> If we can make it work like that on Windows, that's cool. Maybe add
> -DUSE_VCPKG=C:\vcpkg\some-magic-triplet\installed? I don't think there's
> a sane way to guess the triplet. Ah, vcpkg probably has better support
> for all that.
>
> Yeah, back when I started porting Subversion to Windows, and/or building
> Serf there, there was no vcpkg. Getting the dependencies was ... well,
> let's say it was quite the adventure. I had to build *everything* from
> source. If you've never tried to build OpenSSL 0.9 with its Perl-based
> Makefile generator, you haven't lived yet. :D
>
> -- Brane
Hi again,
I've tested it. Here is my feedback.
Both apr-1 and apr-2 works perfectly on a Unix environment.
However, again, dependency search is done in the wrong way on Windows and
may work incorrectly in some cases. In cmake, find_library()
and find_path() are made so they handle all logic in relation to where to
look for each kind of artifact. They will handle system dirs,
-DCMAKE_INSTALL_DIR, -DCMAKE_PREFIX_PATH, <MODULENAME>_ROOT (APR_ROOT in
this case), and even cmake's environment variables, and I guess vcpkg also
implements some custom things. This functionality is very useful for people
who need to set up their environments, to get builds against right
libraries etc...
I would like to suggest fix with the serf-improve-apr-search.patch patch:
[[[
cmake: Improve search of APR and APR-Util libraries by
calling find_library()
and find_path() methods without passing PATH, since cmake could
automatically
determine it based on platform-dependent factors.
]]]
After this patch, I got configuration successfully without modifying
settings on Windows which is good for now...
--
Timofei Zhakov
Index: build/FindAPR.cmake
===================================================================
--- build/FindAPR.cmake (revision 1926089)
+++ build/FindAPR.cmake (working copy)
@@ -106,8 +106,7 @@
# Windows: Find the DLL (runtime) library
function(_apru_find_dll _varname _dllname)
set(CMAKE_FIND_LIBRARY_SUFFIXES ".dll")
- find_library(${_varname} NAMES ${_dllname}
- PATHS ${ARGN} NO_DEFAULT_PATH PATH_SUFFIXES "bin" "lib")
+ find_library(${_varname} NAMES ${_dllname} PATHS ${ARGN} "bin" "lib")
endfunction(_apru_find_dll)
# Extract the main and extra static libraries
@@ -171,21 +170,15 @@
set(APR_FOUND FALSE)
if(${CMAKE_SYSTEM_NAME} MATCHES "Windows")
-
- if(DEFINED APR_ROOT)
- get_filename_component(APR_ROOT "${APR_ROOT}" REALPATH)
- else()
- message(FATAL_ERROR "APR_ROOT must be defined on Windows")
- endif()
-
include(CheckIncludeFile)
find_path(APR_INCLUDES "apr.h"
- PATHS "${APR_ROOT}/include"
- PATH_SUFFIXES "apr-2" "apr-1"
- NO_DEFAULT_PATH)
+ PATH_SUFFIXES
+ "include/apr-2"
+ "include/apr-1"
+ "include")
if(NOT APR_INCLUDES)
- message(FATAL_ERROR "apr.h was not found in ${APR_ROOT}")
+ message(FATAL_ERROR "apr.h was not found")
endif()
if(NOT EXISTS "${APR_INCLUDES}/apr_version.h")
@@ -195,10 +188,10 @@
_apru_version(APR_VERSION _apr_major _apr_minor
"${APR_INCLUDES}/apr_version.h" "APR")
set(_apr_name "apr-${_apr_major}")
- find_library(APR_LIBRARIES NAMES "lib${_apr_name}.lib"
- PATHS ${APR_ROOT} NO_DEFAULT_PATH PATH_SUFFIXES "lib")
- find_library(APR_STATIC_LIBS NAMES "${_apr_name}.lib"
- PATHS ${APR_ROOT} NO_DEFAULT_PATH PATH_SUFFIXES "lib")
+ find_library(APR_LIBRARIES NAMES "lib${_apr_name}"
+ PATH_SUFFIXES "lib")
+ find_library(APR_STATIC_LIBS NAMES "${_apr_name}"
+ PATH_SUFFIXES "lib")
_apru_find_dll(APR_RUNTIME_LIBS "lib${_apr_name}.dll" ${APR_ROOT})
else() # NOT Windows
Index: build/FindAPRUtil.cmake
===================================================================
--- build/FindAPRUtil.cmake (revision 1926089)
+++ build/FindAPRUtil.cmake (working copy)
@@ -78,21 +78,13 @@
unset(_apru_include_only_utilities)
if(${CMAKE_SYSTEM_NAME} MATCHES "Windows")
-
- if(DEFINED APRUtil_ROOT)
- get_filename_component(APRUtil_ROOT "${APRUtil_ROOT}" REALPATH)
- else()
- message(FATAL_ERROR "APRUtil_ROOT must be defined on Windows")
- endif()
-
include(CheckIncludeFile)
find_path(APRUTIL_INCLUDES "apu.h"
- PATHS "${APRUtil_ROOT}/include"
- PATH_SUFFIXES "apr-1"
- NO_DEFAULT_PATH)
+ PATH_SUFFIXES "include/apr-1"
+ PATH_SUFFIXES "include")
if(NOT APRUTIL_INCLUDES)
- message(FATAL_ERROR "apu.h was not found in ${APRUtil_ROOT}")
+ message(FATAL_ERROR "apu.h was not found")
endif()
if(NOT EXISTS "${APRUTIL_INCLUDES}/apu_version.h")
@@ -109,11 +101,11 @@
endif()
find_library(APRUTIL_LIBRARIES NAMES "lib${_apu_name}.lib"
- PATHS ${APRUtil_ROOT} NO_DEFAULT_PATH PATH_SUFFIXES "lib")
+ PATH_SUFFIXES "lib")
find_library(_apu_static NAMES "${_apu_name}.lib"
- PATHS ${APRUtil_ROOT} NO_DEFAULT_PATH PATH_SUFFIXES "lib")
+ PATH_SUFFIXES "lib")
find_library(_apu_expat NAMES ${_apu_expat_name}
- PATHS ${APRUtil_ROOT} NO_DEFAULT_PATH PATH_SUFFIXES "lib")
+ PATH_SUFFIXES "lib")
_apru_find_dll(APRUTIL_RUNTIME_LIBS "lib${_apu_name}.dll" ${APRUtil_ROOT})
if(NOT _apu_expat AND (_apu_expat_name MATCHES "expat"))
