| Message ID | dbaad4753c156487e91b766ebd3c9a39d68c1b12.1637158762.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Upstreaming the Scalar command | expand |
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) > > 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() Just some food for thought, Matthew Rogers
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
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() + parse_makefile_for_executables(git_builtin_extra "BUILT_INS") option(SKIP_DASHED_BUILT_INS "Skip hardlinking the dashed versions of the built-ins") @@ -953,6 +960,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() + #options for configuring test options option(PERL_TESTS "Perform tests that use perl" ON) option(PYTHON_TESTS "Perform tests that use python" ON)