On Thu, Jun 02 2022, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <johannes.schindelin@xxxxxx>
>
> Unlike the `Makefile`-based build, the CMake configuration unfortunately
> does not let us easily encapsulate Scalar's build definition in the
> `contrib/scalar/` subdirectory: The `scalar` executable needs to link in
> `libgit.a` and `common-main.o`, for example.
Are you using "encapsulation" here synonymously with "[mostly] lives in
a different file"? If not I don't really see what distinction you're
trying to draw here.
The contrib/scalar/Makefile "shells out" to the top-level MAkefile to
build libgit.a & common-main.o, as well as scalar.o itself.
> To alleviate that lack of encapsulation somewhat, we guard the Scalar
> parts in `CMakeLists.txt` via the `INCLUDE_SCALAR` environment variable.
> This not only allows the CMake-based build to exclude Scalar by default,
> but also gives better visual cues as to which sections are related to
> Scalar.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@xxxxxx>
> ---
> contrib/buildsystems/CMakeLists.txt | 14 ++++++++++++++
> 1 file changed, 14 insertions(+)
>
> diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt
> index 185f56f414f..c8a802463ba 100644
> --- a/contrib/buildsystems/CMakeLists.txt
> +++ b/contrib/buildsystems/CMakeLists.txt
> @@ -753,6 +753,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()
> +
> parse_makefile_for_executables(git_builtin_extra "BUILT_INS")
>
> option(SKIP_DASHED_BUILT_INS "Skip hardlinking the dashed versions of the built-ins")
> @@ -980,6 +987,13 @@ string(REPLACE "@@BUILD_DIR@@" "${CMAKE_BINARY_DIR}" content "${content}")
> string(REPLACE "@@PROG@@" "git-cvsserver" content "${content}")
> file(WRITE ${CMAKE_BINARY_DIR}/bin-wrappers/git-cvsserver ${content})
>
> +if(DEFINED ENV{INCLUDE_SCALAR} AND NOT ENV{INCLUDE_SCALAR} STREQUAL "")
> + file(STRINGS ${CMAKE_SOURCE_DIR}/wrap-for-bin.sh content NEWLINE_CONSUME)
> + string(REPLACE "@@BUILD_DIR@@" "${CMAKE_BINARY_DIR}" content "${content}")
> + string(REPLACE "@@PROG@@" "contrib/scalar/scalar${EXE_EXTENSION}" content "${content}")
> + file(WRITE ${CMAKE_BINARY_DIR}/bin-wrappers/scalar ${content})
> +endif()
We've been over this before direction we should take to fully integrate
scalar (see [1]), but leaving that aside this seems by any definition to
be more "encapsulated" than what we have in Makefile &
contrib/scalar/Makefile. I.e. this is isolated in its own sections,
whereas in the Makefile case we have the top-level building scalar,
contrib/scalar/Makefile shelling out to it, and then adding its own
behavior etc.
I don't think there's much point to re-visit the discussions around the
time of [1], clearly you feel differently,
But just on the commit message it seems to have been drafted from some
earlier version.
Perhaps it was based on the earlier version of the scalar series before
I'd pointed out the Makefile dependency issues around scalar.o (which
led you to have contrib/scalar/MAkefile "shell out")?
1. https://lore.kernel.org/git/patch-1.1-86fb8d56307-20211028T185016Z-avarab@xxxxxxxxx/
