[geos-devel] cmake detailed comments

[Paul Ramsey]

Thanks for taking the time to look closely, I have rolled some of the
easier notes in and saved others for the next major round.

On Mon, Oct 11, 2021 at 6:10 PM Greg Troxel <gdt at lexort.com> wrote:
>
>
> As this is my first release using cmake I'm reading INSTALL etc. and
> have a few comments.
>
> 0) INSTALL says;
>
>       mkdir build && cd build && cmake -DCMAKE_BUILD_TYPE=Release ..
>
>   Setting `CMAKE_BUILD_TYPE` to `Release` is necessary to enable compiler
>   optimizations.
>
> but CMakeLists.txt says:
>
>   # Default to release build so packagers don't release debug builds
>   set(DEFAULT_BUILD_TYPE Release)
>
>   [set CMAKE_BUILD_TYPE from DEFAULT]
>
> It seems to me that it's best if building without options builds what
> users ought to run.  Perhaps this is changed back and forth, but it
> seems better to expect people testing who want non-optimized builds for
> debug clarity to use a wrapper script to build it that way.  (I tend to
> have a build script for things I build from git, since I need to set up
> prefix and LDFLAGS anyway.)

We do default to release, I just want the doco to be explicit. I
personally forget about CMAKE_BUILD_TYPE all the time and then wonder
where my symbols are.

> CMAKE_BUILD_DEVELOPER seems to be default on.
>
> 1) Doing a build without CMAKE_BUILD_TYPE gets me (among other things)
>   -- GEOS: Using default build type: Release
>   -- GEOS: Developer mode ENABLED
>   -- GEOS: Build astyle ON

I toyed with changing this, but it's actually too big a thing to flip
right before RC. The flags it flips are simultaneously not important
(mostly warnings levels) and kind of important (things one wants
enforced) so... I really don't want devs to work without those flags
under them, and they don't actually *harm* the building of Release
libraries, so I'm leaning to just stripping the option next release
and leaving the flags in by default.
>
> 2) not about cmake but this seems to slow down geos a lot to make tests
> repeatable even though I would guess the answers obtained with extended
> floats[q aren't really wrong, just different -- unless there is some
> "geos computes according to IEEE754" spec, in which case it would be
> nice if the comment explained that instead of talking about making tests
> pass.
>
>   # Use -ffloat-store for 32-bit builds (needed to make some tests pass)
>   target_compile_options(geos_cxx_flags INTERFACE
>     $<$<AND:$<CXX_COMPILER_ID:GNU>,$<EQUAL:4,${CMAKE_SIZEOF_VOID_P}>>:-ffloat-store>
>   )

Good doco point.

> 3) ryu.  I am not able to follow this at all.

It's the double formatter, and Dan had to do some special stuff to get
it to (a) cleanly vendor in but (b) not raise warnings when built at
our warning level. So it's a little less clear than it might otherwise
be.

> 4) tests in top-level
>
> The tests are written as if they are regular code; tests in general need
> to be marked for special linking.  I don't see anything special there
> either.

I don't know what you mean, but I also like how our tests work now so...? :0)

>
> 5) complexities of RPATH handling
>
> I found the following, which I don't fully understand yet:
>
>   https://dev.my-gate.net/2021/08/04/understanding-rpath-with-cmake/
>   https://gitlab.kitware.com/cmake/community/-/wikis/doc/cmake/RPATH-handling
>   https://cmake.cmake.narkive.com/6f4X1K39/change-rpath-at-the-packaging-stage
>
> and not necessarily just about cmake but:
>
>   https://docs.conan.io/en/latest/howtos/manage_shared_libraries/rpaths.html

I hope you'll find out how to make rpaths dance for your platform.


> 6) non-pkgsrc build

I'm not sure what you did or what I can learn from this note, but
thanks again for trying many combinations of things.

> I did a build using cmake from the command line, but aligned with how
> one would build on a (normal ELF, not mac) system that puts software in
> a prefix that isn't part of the default search path, and thus passing
> CPPFLAGS and LDFLAGS for that prefix.  I realize geos is special in that
> it only depends on libc and threads (and perhaps some doc programs), but
> the installed geosop and library need to have a DT_RPATH section
> pointing to $prefix/lib.
>
> Doing that, the tests failed, differently from under pkgsrc, but the
> installed libgeos_c was found and it ended up with both libgeos 3.9 and
> 3.10 both.   The reason is that the test program had both the passed-in
> /usr/pkg/lib RPATH as well as the RPATH from the build directory, but
> the build directory one was after the one intended for install, not
> before.
>
> I think if the build-tree RPATH used for test programs was put before
> the LDFLAGS-derived RPATH, things would be better.
>
> 7) RPATH not used at install time if not in LDFLAGS

> After building for prefix=/usr/pkg, and not passing in LDFLAGS, I did
>
>   make DESTDIR=../destdir install
>
> and got the expected files under destdir, parallel to build.  All of
> geosop, libgeos_c, and geos.pc were missing RPATH.
>
> So probably this is ok, as I realize opinions vary between platforms
> about RPATH.
>
> 8) RPATH used at install time if passed via LDFLAGS
>
> I did a build with the right values for my situation:
>
>   CPPFLAGS=-I/usr/pkg/include LDFLAGS="-L/usr/pkg/lib -Wl,-R/usr/pkg/lib" \
>
> and the (destdir) installed geosop, in objdump -x:
>
>   NEEDED               libgeos_c.so.1
>   NEEDED               libgeos.so.3.10.0
>   NEEDED               libstdc++.so.9
>   NEEDED               libm.so.0
>   NEEDED               libgcc_s.so.1
>   NEEDED               libc.so.12
>   RPATH                /usr/pkg/lib
>
> which looks right.
>
> However geos.pc is missing the -Wl,-R:
>   Libs: -L${libdir} -lgeos_c
>
> Perhaps I'm supposed to be doing something different, but I haven't been
> able to understand what yet.

Thanks for continuing to investigate!

P


> _______________________________________________
> geos-devel mailing list
> geos-devel at lists.osgeo.org
> https://lists.osgeo.org/mailman/listinfo/geos-devel