CMake: improvements so GEOS works as a subproject

[rcoup]

Currently GEOS doesn't work as a subproject in CMake via add_subdirectory() or FetchContent.

Example hello-world super-project & current output.

This is a series of fixes that move GEOS from erroring during generation to building & linking as a CMake subproject:

1. stops using CMAKE_SOURCE_DIR/CMAKE_BINARY_DIR since they're relative to the top-level project which may not be GEOS.
2. expose GEOS::geos_c and GEOS::geos alias targets, which is what find_package(GEOS CONFIG) has already
3. don't setup testing, packaging, developer-mode, etc when GEOS is a subproject. Moves the project() call as early as possible to use PROJECT_IS_TOP_LEVEL for this.
4. fix geos_c headers needing geos headers when using GEOS from the build tree

I'm not a CMake guru, but 1-3 are recommended practises from Professional CMake. I haven't looked at any subproject install issues yet.

[mwtoews]

This looks like a useful improvement! A few remarks:

• Everything works perfectly, and it didn't before!
• It's missing coverage in CI. This can wait until later, however. With PROJ this is done within a script that runs on several CI jobs. Possibly a different strategy would be needed for geos.
• The PROJECT_IS_TOP_LEVEL variable looks useful. But, I wonder if the scope of the broad if(PROJECT_IS_TOP_LEVEL) blocks should be restricted a bit more, e.g. if(PROJECT_IS_TOP_LEVEL AND BUILD_BENCHMARKS)?
• The install target for a subproject will install 471 files from geos. Is this expected? Should a subset of files be installed, e.g. just lib? But not bin/geos-config, include, lib/cmake or lib/pkgconfig?

[rcoup]

The PROJECT_IS_TOP_LEVEL variable looks useful. But, I wonder if the scope of the broad if(PROJECT_IS_TOP_LEVEL) blocks should be restricted a bit more, e.g. if(PROJECT_IS_TOP_LEVEL AND BUILD_BENCHMARKS)?

So you're thinking just repeat the conditional for every relevant section? eg:

#-----------------------------------------------------------------------------
# Tests
#-----------------------------------------------------------------------------
if(PROJECT_IS_TOP_LEVEL)
  include(CTest)
  if(BUILD_TESTING)
    add_subdirectory(tests)
  endif()
endif()

#-----------------------------------------------------------------------------
# Benchmarks
#-----------------------------------------------------------------------------
if(PROJECT_IS_TOP_LEVEL AND BUILD_BENCHMARKS)
  add_subdirectory(benchmarks)
endif()

#-----------------------------------------------------------------------------
# Utils
#-----------------------------------------------------------------------------
if(PROJECT_IS_TOP_LEVEL)
  add_subdirectory(util)
endif()

Quite happy to do that if you think it reads more clearly.

The install target for a subproject will install 471 files from geos. Is this expected? Should a subset of files be installed, e.g. just lib? But not bin/geos-config, include, lib/cmake or lib/pkgconfig?

Yeah, I haven't looked at any subproject install issues yet. My CMake super-project conversion hasn't got that far yet :-)

In principle I'd suggest just the libraries. Super-projects can always reach in and grab other things or point to them. I was planning a followup-PR once I'd got it all working to improve the install aspect.

[rcoup]

It's missing coverage in CI. This can wait until later, however. With PROJ this is done within a script that runs on several CI jobs. Possibly a different strategy would be needed for geos.

I figured downstream people will complain if it breaks - currently they can't use it at all 😄 But yeah, I think a simple super-project in CI probably isn't hard to do — checking that only relevant stuff is installed (ideally and that irrelevant stuff isn't built), making sure CTest doesn't run, etc.

[mwtoews]

Re: repeating the PROJECT_IS_TOP_LEVEL conditional for specific sections, yes I think this might look a bit better. This way it can be more clear and granular which components the subproject logic applies. Perhaps that could do for a first PR.

Agree that further work would be to modify some of the install targets to include/exclude specific components that should be expected with a subproject.

As for the CI side, usually I'd like to see this tested somehow, as it's unfortunate when a new release breaks something downstream. I can probably take care of this at some stage, as I'm keen to eventually see these broader coverages (including post-install tests).

[rcoup]

Re: repeating the PROJECT_IS_TOP_LEVEL conditional for specific sections, yes I think this might look a bit better. This way it can be more clear and granular which components the subproject logic applies. Perhaps that could do for a first PR.

Done.

As for the CI side, usually I'd like to see this tested somehow, as it's unfortunate when a new release breaks something downstream. I can probably take care of this at some stage, as I'm keen to eventually see these broader coverages (including post-install tests).

I added a simple GH Actions workflow to at least check it builds as a subproject, and that the tests aren't built. Is a starting point at least.

[rcoup]

@mwtoews let me know if you want any further changes here

[mwtoews]

Thanks for the updates, this looks excellent! There are no conflicts to any other aspects of the build, and obviously fixes the library to be used as a sub-project.

I'll let this wait for a few days if anyone else wants to weigh-in, but will otherwise use a "merge commit" to preserve the individual commits.

[mwtoews]

Thanks again @rcoup

A "merge commit" was not enabled for this repo (to my supprise), so I edited the "squash and merge" commit with the primary component from each message.

[SpaceIm]

Sorry to drop a review 7 months after the PR, but I see these modifications with the new release. Actually this block is useless and misleading, geos_c doesn't have any interface dependency to geos headers, just private usage of geos headers handled by the link of geos target in top CMakeLists.

Actually, everything can be simplified with:

target_include_directories(geos_c PUBLIC $<BUILD_INTERFACE:${CMAKE_CURRENT_BINARY_DIR}>)

and it was already in this CMakeLists

[rcoup]

It's 7 months later, so I'll need to go and dig it out again... but something like this was clearly needed for it to build/work as a subproject, otherwise it wouldn't have been here 😃

As a guess, I think because pre-install when building the super-project source/include/ is needed, since install/include/geos isn't populated yet?