| Message ID | pull.970.v2.git.1622980974.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Make CMake work out of the box | expand |
"Matthew Rogers via GitGitGadget" <gitgitgadget@gmail.com> writes: > This pull request comes from our discussion here[1], and I think these > patches provide a good compromise around the concerns discussed there > > 1: > https://lore.kernel.org/git/CAOjrSZusMSvs7AS-ZDsV8aQUgsF2ZA754vSDjgFKMRgi_oZAWw@mail.gmail.com/ > > CCing the people involved in the original discussion. cc: Philip Oakley > philipoakley@iee.email cc: Sibi Siddharthan > sibisiddharthan.github@gmail.com, cc: Johannes Schindelin > johannes.schindelin@gmx.de, cc: Danh Doan congdanhqx@gmail.com > > Matthew Rogers (3): > cmake: add knob to disable vcpkg > cmake: create compile_commands.json by default > cmake: add warning for ignored MSGFMT_EXE I am neither cmake nor windows person, so I'll queue this as-is and wait for the stakeholders to chime in. I did wonder if we want this to be applicable to the maintenance track for 2.31, though. There is a textual conflict with the addition of SIMPLE_IPC that happened during 2.32 cycle, which is easily resolvable. I am tempted to queue a version of these three patches rebased on to 'maint' after making sure that the result of merging that into 'master' is byte-for-byte identical to applying these three patches directly on to 'master'. The range-diff looks like the attached. Thanks. 1: 546c49cc88 ! 1: 585b7ca371 cmake: add knob to disable vcpkg @@ contrib/buildsystems/CMakeLists.txt: endif() if(NOT EXISTS ${MSGFMT_EXE}) message(WARNING "Text Translations won't be built") unset(MSGFMT_EXE) -@@ contrib/buildsystems/CMakeLists.txt: file(APPEND ${CMAKE_BINARY_DIR}/GIT-BUILD-OPTIONS "X='${EXE_EXTENSION}'\n") - file(APPEND ${CMAKE_BINARY_DIR}/GIT-BUILD-OPTIONS "NO_GETTEXT='${NO_GETTEXT}'\n") +@@ contrib/buildsystems/CMakeLists.txt: file(APPEND ${CMAKE_BINARY_DIR}/GIT-BUILD-OPTIONS "NO_GETTEXT='${NO_GETTEXT}'\n" file(APPEND ${CMAKE_BINARY_DIR}/GIT-BUILD-OPTIONS "RUNTIME_PREFIX='${RUNTIME_PREFIX}'\n") file(APPEND ${CMAKE_BINARY_DIR}/GIT-BUILD-OPTIONS "NO_PYTHON='${NO_PYTHON}'\n") + file(APPEND ${CMAKE_BINARY_DIR}/GIT-BUILD-OPTIONS "SUPPORTS_SIMPLE_IPC='${SUPPORTS_SIMPLE_IPC}'\n") -if(WIN32) +if(USE_VCPKG) file(APPEND ${CMAKE_BINARY_DIR}/GIT-BUILD-OPTIONS "PATH=\"$PATH:$TEST_DIRECTORY/../compat/vcbuild/vcpkg/installed/x64-windows/bin\"\n") 2: efa8681a22 = 2: 1cba2f9bd1 cmake: create compile_commands.json by default 3: ceeca2bc0d = 3: 7824e74976 cmake: add warning for ignored MSGFMT_EXE
Hi Junio, On Mon, 7 Jun 2021, Junio C Hamano wrote: > "Matthew Rogers via GitGitGadget" <gitgitgadget@gmail.com> writes: > > > This pull request comes from our discussion here[1], and I think these > > patches provide a good compromise around the concerns discussed there > > > > 1: > > https://lore.kernel.org/git/CAOjrSZusMSvs7AS-ZDsV8aQUgsF2ZA754vSDjgFKMRgi_oZAWw@mail.gmail.com/ > > > > CCing the people involved in the original discussion. cc: Philip Oakley > > philipoakley@iee.email cc: Sibi Siddharthan > > sibisiddharthan.github@gmail.com, cc: Johannes Schindelin > > johannes.schindelin@gmx.de, cc: Danh Doan congdanhqx@gmail.com > > > > Matthew Rogers (3): > > cmake: add knob to disable vcpkg > > cmake: create compile_commands.json by default > > cmake: add warning for ignored MSGFMT_EXE > > I am neither cmake nor windows person, so I'll queue this as-is and > wait for the stakeholders to chime in. As long as the CI builds pass, I am in favor of integrating the patch series. > I did wonder if we want this to be applicable to the maintenance > track for 2.31, though. There is a textual conflict with the > addition of SIMPLE_IPC that happened during 2.32 cycle, which is > easily resolvable. If it isn't much work, sure. But I would think that developers who want to build using Visual Studio really should stay on newer branches. Thanks, Dscho > I am tempted to queue a version of these three patches rebased on to > 'maint' after making sure that the result of merging that into > 'master' is byte-for-byte identical to applying these three patches > directly on to 'master'. > > The range-diff looks like the attached. Thanks. > > 1: 546c49cc88 ! 1: 585b7ca371 cmake: add knob to disable vcpkg > @@ contrib/buildsystems/CMakeLists.txt: endif() > if(NOT EXISTS ${MSGFMT_EXE}) > message(WARNING "Text Translations won't be built") > unset(MSGFMT_EXE) > -@@ contrib/buildsystems/CMakeLists.txt: file(APPEND ${CMAKE_BINARY_DIR}/GIT-BUILD-OPTIONS "X='${EXE_EXTENSION}'\n") > - file(APPEND ${CMAKE_BINARY_DIR}/GIT-BUILD-OPTIONS "NO_GETTEXT='${NO_GETTEXT}'\n") > +@@ contrib/buildsystems/CMakeLists.txt: file(APPEND ${CMAKE_BINARY_DIR}/GIT-BUILD-OPTIONS "NO_GETTEXT='${NO_GETTEXT}'\n" > file(APPEND ${CMAKE_BINARY_DIR}/GIT-BUILD-OPTIONS "RUNTIME_PREFIX='${RUNTIME_PREFIX}'\n") > file(APPEND ${CMAKE_BINARY_DIR}/GIT-BUILD-OPTIONS "NO_PYTHON='${NO_PYTHON}'\n") > + file(APPEND ${CMAKE_BINARY_DIR}/GIT-BUILD-OPTIONS "SUPPORTS_SIMPLE_IPC='${SUPPORTS_SIMPLE_IPC}'\n") > -if(WIN32) > +if(USE_VCPKG) > file(APPEND ${CMAKE_BINARY_DIR}/GIT-BUILD-OPTIONS "PATH=\"$PATH:$TEST_DIRECTORY/../compat/vcbuild/vcpkg/installed/x64-windows/bin\"\n") > 2: efa8681a22 = 2: 1cba2f9bd1 cmake: create compile_commands.json by default > 3: ceeca2bc0d = 3: 7824e74976 cmake: add warning for ignored MSGFMT_EXE >
Hi Matt, On Sun, 6 Jun 2021, Matthew Rogers via GitGitGadget wrote: > This pull request comes from our discussion here[1], and I think these > patches provide a good compromise around the concerns discussed there > > 1: > https://lore.kernel.org/git/CAOjrSZusMSvs7AS-ZDsV8aQUgsF2ZA754vSDjgFKMRgi_oZAWw@mail.gmail.com/ > > CCing the people involved in the original discussion. cc: Philip Oakley > philipoakley@iee.email cc: Sibi Siddharthan > sibisiddharthan.github@gmail.com, cc: Johannes Schindelin > johannes.schindelin@gmx.de, cc: Danh Doan congdanhqx@gmail.com Just in case that a v3 is needed, I fixed the PR description so that these "Cc:"s are interpreted correctly again by GitGitGadget. But from a brief glance over v2, all patches look good to me. Thanks, Dscho > > Matthew Rogers (3): > cmake: add knob to disable vcpkg > cmake: create compile_commands.json by default > cmake: add warning for ignored MSGFMT_EXE > > contrib/buildsystems/CMakeLists.txt | 37 ++++++++++++++++++++++------- > 1 file changed, 28 insertions(+), 9 deletions(-) > > > base-commit: c09b6306c6ca275ed9d0348a8c8014b2ff723cfb > Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-970%2FROGERSM94%2Ffix-cmake-v2 > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-970/ROGERSM94/fix-cmake-v2 > Pull-Request: https://github.com/gitgitgadget/git/pull/970 > > Range-diff vs v1: > > 1: 3170f78daa5f ! 1: 485254b49de8 cmake: add knob to disable vcpkg > @@ Commit message > generators. > > - Some versions of Visual Studio 2019 moved away from using the > - VS 2019 by default, making it impossible for Visual Studio to > - configure the project in the likely event that it couldn't find the > - dependencies. > + VS 2019 generator by default, making it impossible for Visual > + Studio to configure the project in the likely event that it couldn't > + find the dependencies. > > - Inexperienced users of CMake are very likely to get tripped up by > the errors caused by a lack of vcpkg, making the above bullet point > both annoying and hard to debug. > > - As such, lets make using vcpkg the default on windows. Users who want > + As such, let's make using vcpkg the default on windows. Users who want > to avoid using vcpkg can disable it by passing -DNO_VCPKG=TRUE. > > Signed-off-by: Matthew Rogers <mattr94@gmail.com> > @@ contrib/buildsystems/CMakeLists.txt: NOTE: By default CMake uses Makefile as the > set(CMAKE_SOURCE_DIR ${CMAKE_CURRENT_LIST_DIR}/../..) > -if(WIN32) > + > -+if (WIN32 AND NOT NO_VCPKG) > -+ set(USING_VCPKG TRUE) > -+else() > -+ set(USING_VCPKG FALSE) > ++option(USE_VCPKG "Whether or not to use vcpkg for obtaining dependencies. Only applicable to Windows platforms" ON) > ++if(NOT WIN32) > ++ set(USE_VCPKG OFF CACHE BOOL FORCE) > +endif() > + > -+if(USING_VCPKG) > ++if(USE_VCPKG) > set(VCPKG_DIR "${CMAKE_SOURCE_DIR}/compat/vcbuild/vcpkg") > - if(MSVC AND NOT EXISTS ${VCPKG_DIR}) > + if(NOT EXISTS ${VCPKG_DIR}) > @@ contrib/buildsystems/CMakeLists.txt: endif() > find_program(MSGFMT_EXE msgfmt) > if(NOT MSGFMT_EXE) > - set(MSGFMT_EXE ${CMAKE_SOURCE_DIR}/compat/vcbuild/vcpkg/downloads/tools/msys2/msys64/usr/bin/msgfmt.exe) > -+ if (USING_VCPKG) > ++ if (USE_VCPKG) > + set(MSGFMT_EXE ${CMAKE_SOURCE_DIR}/compat/vcbuild/vcpkg/downloads/tools/msys2/msys64/usr/bin/msgfmt.exe) > + endif() > if(NOT EXISTS ${MSGFMT_EXE}) > @@ contrib/buildsystems/CMakeLists.txt: file(APPEND ${CMAKE_BINARY_DIR}/GIT-BUILD-O > file(APPEND ${CMAKE_BINARY_DIR}/GIT-BUILD-OPTIONS "NO_PYTHON='${NO_PYTHON}'\n") > file(APPEND ${CMAKE_BINARY_DIR}/GIT-BUILD-OPTIONS "SUPPORTS_SIMPLE_IPC='${SUPPORTS_SIMPLE_IPC}'\n") > -if(WIN32) > -+if(USING_VCPKG) > ++if(USE_VCPKG) > file(APPEND ${CMAKE_BINARY_DIR}/GIT-BUILD-OPTIONS "PATH=\"$PATH:$TEST_DIRECTORY/../compat/vcbuild/vcpkg/installed/x64-windows/bin\"\n") > endif() > > 2: c3bf266cf03a ! 2: a3b5eef54188 cmake: create compile_commands.json by default > @@ Commit message > time of this writing, and no real negative consequences that I can find > with my search-skills. > > - NOTE: That the comppile_commands.json is currenntly produced only when > + NOTE: That the compile_commands.json is currently produced only when > using the Ninja and Makefile generators. See The CMake documentation[3] > for more info. > > @@ Commit message > Signed-off-by: Matthew Rogers <mattr94@gmail.com> > > ## contrib/buildsystems/CMakeLists.txt ## > -@@ contrib/buildsystems/CMakeLists.txt: else() > - set(USING_VCPKG FALSE) > +@@ contrib/buildsystems/CMakeLists.txt: if(NOT WIN32) > + set(USE_VCPKG OFF CACHE BOOL FORCE) > endif() > > -+if (NOT DEFINED CMAKE_EXPORT_COMPILE_COMMANDS) > -+ SET(CMAKE_EXPORT_COMPILE_COMMANDS TRUE) > ++if(NOT DEFINED CMAKE_EXPORT_COMPILE_COMMANDS) > ++ set(CMAKE_EXPORT_COMPILE_COMMANDS TRUE) > +endif() > + > - if(USING_VCPKG) > + if(USE_VCPKG) > set(VCPKG_DIR "${CMAKE_SOURCE_DIR}/compat/vcbuild/vcpkg") > if(NOT EXISTS ${VCPKG_DIR}) > 3: 07763a9de723 ! 3: 2110c8ffa423 cmake: add warning for ignored MSGFMT_EXE > @@ Commit message > configured, as such add a check for NO_GETTEXT before attempting to set > it. > > - suggested-by: Johannes Schindelin <johannes.schindelin@gmx.de> > + Suggested-by: Johannes Schindelin <johannes.schindelin@gmx.de> > Signed-off-by: Matthew Rogers <mattr94@gmail.com> > > ## contrib/buildsystems/CMakeLists.txt ## > @@ contrib/buildsystems/CMakeLists.txt: if(WIN32 AND NOT MSVC)#not required for vis > > -find_program(MSGFMT_EXE msgfmt) > -if(NOT MSGFMT_EXE) > -- if (USING_VCPKG) > +- if (USE_VCPKG) > - set(MSGFMT_EXE ${CMAKE_SOURCE_DIR}/compat/vcbuild/vcpkg/downloads/tools/msys2/msys64/usr/bin/msgfmt.exe) > - endif() > - if(NOT EXISTS ${MSGFMT_EXE}) > @@ contrib/buildsystems/CMakeLists.txt: if(WIN32 AND NOT MSVC)#not required for vis > +else() > + find_program(MSGFMT_EXE msgfmt) > + if(NOT MSGFMT_EXE) > -+ if (USING_VCPKG) > ++ if(USE_VCPKG) > + set(MSGFMT_EXE ${CMAKE_SOURCE_DIR}/compat/vcbuild/vcpkg/downloads/tools/msys2/msys64/usr/bin/msgfmt.exe) > + endif() > + if(NOT EXISTS ${MSGFMT_EXE}) > > -- > gitgitgadget >
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > Just in case that a v3 is needed, I fixed the PR description so that these > "Cc:"s are interpreted correctly again by GitGitGadget. > > But from a brief glance over v2, all patches look good to me. Let me amend v2 with your Acked-by and merge it down to 'next', then. Thanks.
On 07/06/2021 01:54, Junio C Hamano wrote: > "Matthew Rogers via GitGitGadget" <gitgitgadget@gmail.com> writes: > >> This pull request comes from our discussion here[1], and I think these >> patches provide a good compromise around the concerns discussed there >> >> 1: >> https://lore.kernel.org/git/CAOjrSZusMSvs7AS-ZDsV8aQUgsF2ZA754vSDjgFKMRgi_oZAWw@mail.gmail.com/ >> >> CCing the people involved in the original discussion. cc: Philip Oakley >> philipoakley@iee.email cc: Sibi Siddharthan >> sibisiddharthan.github@gmail.com, cc: Johannes Schindelin >> johannes.schindelin@gmx.de, cc: Danh Doan congdanhqx@gmail.com >> >> Matthew Rogers (3): >> cmake: add knob to disable vcpkg >> cmake: create compile_commands.json by default >> cmake: add warning for ignored MSGFMT_EXE > I am neither cmake nor windows person, so I'll queue this as-is and > wait for the stakeholders to chime in. > > I did wonder if we want this to be applicable to the maintenance > track for 2.31, though. There is a textual conflict with the > addition of SIMPLE_IPC that happened during 2.32 cycle, which is > easily resolvable. > > I am tempted to queue a version of these three patches rebased on to > 'maint' after making sure that the result of merging that into > 'master' is byte-for-byte identical to applying these three patches > directly on to 'master'. Sorry for the delay - I've been off-line and I'm only now catching up. Could we confirm that the CI actually tests the update. IIRC the yml setup preloaded the vcpkg artefacts that this change looks to make work 'out of the box'. Philip
On 10/06/2021 10:45, Johannes Schindelin wrote: > As long as the CI builds pass, I am in favor of integrating the patch > series. > >> I did wonder if we want this to be applicable to the maintenance >> track for 2.31, though. There is a textual conflict with the >> addition of SIMPLE_IPC that happened during 2.32 cycle, which is >> easily resolvable. > If it isn't much work, sure. But I would think that developers who want to > build using Visual Studio really should stay on newer branches. Ack. Philip
This pull request comes from our discussion here[1], and I think these patches provide a good compromise around the concerns discussed there 1: https://lore.kernel.org/git/CAOjrSZusMSvs7AS-ZDsV8aQUgsF2ZA754vSDjgFKMRgi_oZAWw@mail.gmail.com/ CCing the people involved in the original discussion. cc: Philip Oakley philipoakley@iee.email cc: Sibi Siddharthan sibisiddharthan.github@gmail.com, cc: Johannes Schindelin johannes.schindelin@gmx.de, cc: Danh Doan congdanhqx@gmail.com Matthew Rogers (3): cmake: add knob to disable vcpkg cmake: create compile_commands.json by default cmake: add warning for ignored MSGFMT_EXE contrib/buildsystems/CMakeLists.txt | 37 ++++++++++++++++++++++------- 1 file changed, 28 insertions(+), 9 deletions(-) base-commit: c09b6306c6ca275ed9d0348a8c8014b2ff723cfb Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-970%2FROGERSM94%2Ffix-cmake-v2 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-970/ROGERSM94/fix-cmake-v2 Pull-Request: https://github.com/gitgitgadget/git/pull/970 Range-diff vs v1: 1: 3170f78daa5f ! 1: 485254b49de8 cmake: add knob to disable vcpkg @@ Commit message generators. - Some versions of Visual Studio 2019 moved away from using the - VS 2019 by default, making it impossible for Visual Studio to - configure the project in the likely event that it couldn't find the - dependencies. + VS 2019 generator by default, making it impossible for Visual + Studio to configure the project in the likely event that it couldn't + find the dependencies. - Inexperienced users of CMake are very likely to get tripped up by the errors caused by a lack of vcpkg, making the above bullet point both annoying and hard to debug. - As such, lets make using vcpkg the default on windows. Users who want + As such, let's make using vcpkg the default on windows. Users who want to avoid using vcpkg can disable it by passing -DNO_VCPKG=TRUE. Signed-off-by: Matthew Rogers <mattr94@gmail.com> @@ contrib/buildsystems/CMakeLists.txt: NOTE: By default CMake uses Makefile as the set(CMAKE_SOURCE_DIR ${CMAKE_CURRENT_LIST_DIR}/../..) -if(WIN32) + -+if (WIN32 AND NOT NO_VCPKG) -+ set(USING_VCPKG TRUE) -+else() -+ set(USING_VCPKG FALSE) ++option(USE_VCPKG "Whether or not to use vcpkg for obtaining dependencies. Only applicable to Windows platforms" ON) ++if(NOT WIN32) ++ set(USE_VCPKG OFF CACHE BOOL FORCE) +endif() + -+if(USING_VCPKG) ++if(USE_VCPKG) set(VCPKG_DIR "${CMAKE_SOURCE_DIR}/compat/vcbuild/vcpkg") - if(MSVC AND NOT EXISTS ${VCPKG_DIR}) + if(NOT EXISTS ${VCPKG_DIR}) @@ contrib/buildsystems/CMakeLists.txt: endif() find_program(MSGFMT_EXE msgfmt) if(NOT MSGFMT_EXE) - set(MSGFMT_EXE ${CMAKE_SOURCE_DIR}/compat/vcbuild/vcpkg/downloads/tools/msys2/msys64/usr/bin/msgfmt.exe) -+ if (USING_VCPKG) ++ if (USE_VCPKG) + set(MSGFMT_EXE ${CMAKE_SOURCE_DIR}/compat/vcbuild/vcpkg/downloads/tools/msys2/msys64/usr/bin/msgfmt.exe) + endif() if(NOT EXISTS ${MSGFMT_EXE}) @@ contrib/buildsystems/CMakeLists.txt: file(APPEND ${CMAKE_BINARY_DIR}/GIT-BUILD-O file(APPEND ${CMAKE_BINARY_DIR}/GIT-BUILD-OPTIONS "NO_PYTHON='${NO_PYTHON}'\n") file(APPEND ${CMAKE_BINARY_DIR}/GIT-BUILD-OPTIONS "SUPPORTS_SIMPLE_IPC='${SUPPORTS_SIMPLE_IPC}'\n") -if(WIN32) -+if(USING_VCPKG) ++if(USE_VCPKG) file(APPEND ${CMAKE_BINARY_DIR}/GIT-BUILD-OPTIONS "PATH=\"$PATH:$TEST_DIRECTORY/../compat/vcbuild/vcpkg/installed/x64-windows/bin\"\n") endif() 2: c3bf266cf03a ! 2: a3b5eef54188 cmake: create compile_commands.json by default @@ Commit message time of this writing, and no real negative consequences that I can find with my search-skills. - NOTE: That the comppile_commands.json is currenntly produced only when + NOTE: That the compile_commands.json is currently produced only when using the Ninja and Makefile generators. See The CMake documentation[3] for more info. @@ Commit message Signed-off-by: Matthew Rogers <mattr94@gmail.com> ## contrib/buildsystems/CMakeLists.txt ## -@@ contrib/buildsystems/CMakeLists.txt: else() - set(USING_VCPKG FALSE) +@@ contrib/buildsystems/CMakeLists.txt: if(NOT WIN32) + set(USE_VCPKG OFF CACHE BOOL FORCE) endif() -+if (NOT DEFINED CMAKE_EXPORT_COMPILE_COMMANDS) -+ SET(CMAKE_EXPORT_COMPILE_COMMANDS TRUE) ++if(NOT DEFINED CMAKE_EXPORT_COMPILE_COMMANDS) ++ set(CMAKE_EXPORT_COMPILE_COMMANDS TRUE) +endif() + - if(USING_VCPKG) + if(USE_VCPKG) set(VCPKG_DIR "${CMAKE_SOURCE_DIR}/compat/vcbuild/vcpkg") if(NOT EXISTS ${VCPKG_DIR}) 3: 07763a9de723 ! 3: 2110c8ffa423 cmake: add warning for ignored MSGFMT_EXE @@ Commit message configured, as such add a check for NO_GETTEXT before attempting to set it. - suggested-by: Johannes Schindelin <johannes.schindelin@gmx.de> + Suggested-by: Johannes Schindelin <johannes.schindelin@gmx.de> Signed-off-by: Matthew Rogers <mattr94@gmail.com> ## contrib/buildsystems/CMakeLists.txt ## @@ contrib/buildsystems/CMakeLists.txt: if(WIN32 AND NOT MSVC)#not required for vis -find_program(MSGFMT_EXE msgfmt) -if(NOT MSGFMT_EXE) -- if (USING_VCPKG) +- if (USE_VCPKG) - set(MSGFMT_EXE ${CMAKE_SOURCE_DIR}/compat/vcbuild/vcpkg/downloads/tools/msys2/msys64/usr/bin/msgfmt.exe) - endif() - if(NOT EXISTS ${MSGFMT_EXE}) @@ contrib/buildsystems/CMakeLists.txt: if(WIN32 AND NOT MSVC)#not required for vis +else() + find_program(MSGFMT_EXE msgfmt) + if(NOT MSGFMT_EXE) -+ if (USING_VCPKG) ++ if(USE_VCPKG) + set(MSGFMT_EXE ${CMAKE_SOURCE_DIR}/compat/vcbuild/vcpkg/downloads/tools/msys2/msys64/usr/bin/msgfmt.exe) + endif() + if(NOT EXISTS ${MSGFMT_EXE})