git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH 1/2] cmake: optionally build `scalar`, too

From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date
Jun 2, 2022, 10:18 UTC
Message-ID
<220602.86ilpjz7mz.gmgdl@evledraar.gmail.com>
In-Reply-To
<c155cadfa309e856f13ea7d005870dab706a74ce.1654160735.git.gitgitgadget@gmail.com>
On Thu, Jun 02 2022, Johannes Schindelin via GitGitGadget wrote:
Show 6 quoted lines
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> 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.

Show 39 quoted lines
> 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@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 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@gmail.com/
Previous: Johannes Schindelin via GitGitGadgetNext: Johannes Schindelin via GitGitGadget
Message 3 of 14 in “Integrate Scalar into the CI builds”
  1. 0/2 Integrate Scalar into the CI buildsJohannes Schindelin via GitGitGadget, Jun 2, 2022
  2. 1/2 cmake: optionally build `scalar`, tooJohannes Schindelin via GitGitGadget, Jun 2, 2022
  3. Ævar Arnfjörð BjarmasonJun 2, 2022
  4. 2/2 ci: also run the `scalar` testsJohannes Schindelin via GitGitGadget, Jun 2, 2022
  5. Ævar Arnfjörð BjarmasonJun 2, 2022
  6. Derrick StoleeJun 2, 2022
  7. Johannes SchindelinJun 3, 2022
  8. Derrick StoleeJun 2, 2022
  9. Ævar Arnfjörð BjarmasonJun 2, 2022
  10. Derrick StoleeJun 2, 2022
  11. Johannes SchindelinJun 3, 2022
  12. Ævar Arnfjörð BjarmasonJun 3, 2022
  13. Johannes SchindelinJun 13, 2022
  14. Ævar Arnfjörð BjarmasonJun 23, 2022

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.