Re: [PATCH v7 05/17] cmake: optionally build `scalar`, too

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Matt,

On Wed, 17 Nov 2021, Matt Rogers wrote:

> On Wed, Nov 17, 2021 at 9:49 AM Johannes Schindelin via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
> >
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > The CMake configuration unfortunately does not let us encapsulate all
> > (or at least the vast majority) of Scalar's build definition in the
> > `contrib/scalar/` subdirectory.
>
> I believe that this isn't fully correct in that you could call
> add_subdirectory() with a directory that isn't a subdirectory so long as
> you provide it an explicit binary directory which is part of what we're
> doing here anyways. (at least it works fine for me with cmake version
> 3.21)

You are of course correct, you _could_ put things into a subdirectory with
CMake. However, please note that `scalar` wants to link to `libgit.a` (and
also link in `common-main.o`), therefore the subdirectory _still_ has to
have some sort of connection to the top-level configuration.

I shall clarify that in the commit message in the next iteration.

> > To alleviate that somewhat, we guard the inclusion of Scalar via the
> > `INCLUDE_SCALAR` environment variable.
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> >  contrib/buildsystems/CMakeLists.txt | 14 ++++++++++++++
> >  1 file changed, 14 insertions(+)
> >
> > diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt
> > index fd1399c440f..dd7496b0322 100644
> > --- a/contrib/buildsystems/CMakeLists.txt
> > +++ b/contrib/buildsystems/CMakeLists.txt
> > @@ -729,6 +729,13 @@ if(CURL_FOUND)
> >         endif()
> >  endif()
> >
> > +if(DEFINED ENV{INCLUDE_SCALAR} AND NOT ENV{INCLUDE_SCALAR} STREQUAL "")
> > +       add_executable(scalar ${CMAKE_SOURCE_DIR}/contrib/scalar/scalar.c)
> > +       target_link_libraries(scalar common-main)
> > +       set_target_properties(scalar PROPERTIES RUNTIME_OUTPUT_DIRECTORY_DEBUG ${CMAKE_BINARY_DIR}/contrib/scalar)
> > +       set_target_properties(scalar PROPERTIES RUNTIME_OUTPUT_DIRECTORY_RELEASE ${CMAKE_BINARY_DIR}/contrib/scalar)
> > +endif()
>
> Would it make sense to just mark this with EXCLUDE_FROM_ALL so that way
> you don't have to have the INCLUDE_SCALAR environment variable just for
> this?  This way people who don't want to build scalar can still run
> `cmake --build .` and not have scalar built and people who do want it
> built can just run `cmake --build . --target scalar`, alternatively you
> could also do something like
>
> if(DEFINED ENV{INCLUDE_SCALAR} AND NOT ENV{INCLUDE_SCALAR} STREQUAL "")
>     add_executable(scalar ${CMAKE_SOURCE_DIR}/contrib/scalar/scalar.c)
> else()
>     add_executable(scalar EXCLUDE_FROM_ALL
> ${CMAKE_SOURCE_DIR}/contrib/scalar/scalar.c)
> endif()

The entire point of pulling this patch from a much later patch series to
the first Scalar patch series is to allow for easy testing in the CI
build. To that end, what you propose would complexify the setup too much
for my liking.

Since the end game seems to be for Scalar to become a top-level command
(at least that's how I read Junio's favorable reply at
https://lore.kernel.org/git/xmqq7ddnz115.fsf@gitster.g/), I suspect that
it might not matter whether we stay with `INCLUDE_SCALAR` or change it to
an idempotent alternative, anyway.

Ciao,
Dscho