Message ID | patch-1.1-0fa41115261-20221219T102205Z-avarab@gmail.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | cmake: don't invoke msgfmt with --statistics | expand |
Hi Ævar On 19/12/2022 10:26, Ævar Arnfjörð Bjarmason wrote: > In [1] I made the same change to our Makefile, let's follow-up and do > the same here. > > For "cmake" this is particularly nice with "-G Ninja", as before we'd > emit ~40 lines of overflowed progress bar output, but now it's only > the one line of "ninja"'s progress bar. I don't really have a strong opinion either way on this but if it matches what we do in the Makefile than it sounds sensible. Best Wishes Phillip > 1. 2f12b31b746 (Makefile: don't invoke msgfmt with --statistics, > 2021-12-17) > > Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> > --- > > This trivial fix is extracted from the ab/cmake-nix-and-ci topic which > was ejected around the time of the release for that previous > submission see [1], the range-diff is to that topic. > > I'm re-arranging and re-submitting topic more piecemeal. There were no > outstanding issues or feedback with this part of it, so hopefully this > can advance relatively quickly. > > I'll also submit some of the other uncontroversial bits today > independently, none of which conflict with one another. Then once > those have landed try to find some acceptable way forward for the > later bits, which at that point will be easier to review. > > 1. https://lore.kernel.org/git/cover-v6-00.15-00000000000-20221206T001617Z-avarab@gmail.com/ > > Range-diff: > 1: fc190b379cd = 1: 0fa41115261 cmake: don't invoke msgfmt with --statistics > 2: 1a11aa233a3 < -: ----------- cmake: use "-S" and "-B" to specify source and build directories > 3: b9ddb5db1d3 < -: ----------- cmake: update instructions for portable CMakeLists.txt > 4: 7b7850c00ee < -: ----------- cmake: don't copy chainlint.pl to build directory > 5: 82ecb797915 < -: ----------- cmake: chmod +x the bin-wrappers/* & SCRIPT_{SH,PERL} & git-p4 > 6: 1f326944a07 < -: ----------- cmake & test-lib.sh: add a $GIT_SOURCE_DIR variable > 7: 973c8038f54 < -: ----------- cmake: set "USE_LIBPCRE2" in "GIT-BUILD-OPTIONS" for test-lib.sh > 8: b8448c7a8a6 < -: ----------- Makefile + test-lib.sh: don't prefer cmake-built to make-built git > 9: 5135e40969e < -: ----------- test-lib.sh: support a "GIT_TEST_BUILD_DIR" > 10: 65204463730 < -: ----------- cmake: optionally be able to run tests before "ctest" > 11: e25992b16f1 < -: ----------- cmake: support GIT_TEST_OPTS, abstract away WIN32 defaults > 12: 4905ce5321d < -: ----------- cmake: increase test timeout on Windows only > 13: 6c6b530965d < -: ----------- cmake: only look for "sh" in "C:/Program Files" on Windows > 14: 563f1b9b045 < -: ----------- cmake: copy over git-p4.py for t983[56] perforce test > 15: 917a884eb65 < -: ----------- CI: add a "linux-cmake-test" to run cmake & ctest on linux > > contrib/buildsystems/CMakeLists.txt | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt > index 2f6e0197ffa..8f8b6f375f7 100644 > --- a/contrib/buildsystems/CMakeLists.txt > +++ b/contrib/buildsystems/CMakeLists.txt > @@ -897,7 +897,7 @@ if(MSGFMT_EXE) > foreach(po ${po_files}) > file(MAKE_DIRECTORY ${CMAKE_BINARY_DIR}/po/build/locale/${po}/LC_MESSAGES) > add_custom_command(OUTPUT ${CMAKE_BINARY_DIR}/po/build/locale/${po}/LC_MESSAGES/git.mo > - COMMAND ${MSGFMT_EXE} --check --statistics -o ${CMAKE_BINARY_DIR}/po/build/locale/${po}/LC_MESSAGES/git.mo ${CMAKE_SOURCE_DIR}/po/${po}.po) > + COMMAND ${MSGFMT_EXE} --check -o ${CMAKE_BINARY_DIR}/po/build/locale/${po}/LC_MESSAGES/git.mo ${CMAKE_SOURCE_DIR}/po/${po}.po) > list(APPEND po_gen ${CMAKE_BINARY_DIR}/po/build/locale/${po}/LC_MESSAGES/git.mo) > endforeach() > add_custom_target(po-gen ALL DEPENDS ${po_gen})
Phillip Wood <phillip.wood123@gmail.com> writes: > On 19/12/2022 10:26, Ævar Arnfjörð Bjarmason wrote: >> In [1] I made the same change to our Makefile, let's follow-up and do >> the same here. >> For "cmake" this is particularly nice with "-G Ninja", as before >> we'd >> emit ~40 lines of overflowed progress bar output, but now it's only >> the one line of "ninja"'s progress bar. > > I don't really have a strong opinion either way on this but if it > matches what we do in the Makefile than it sounds sensible. As a one-shot change, it might be sensible to claim consistency by saying "we do the same thing in two places", but I'd worry more about the root cause of such inconsistency in the first place, i.e. can we have some trick to ensure that two build systems will not reimplement the same thing slightly differently? It also is worth examining if having "the same change" is a good idea in the first place. The justification given "In [1]" was that a build driven by our Makefile were concise and non-verbose overall, but with --stat that concise output pattern was broken. I do not know (and I do not have particular interest in knowing) how a build driven by cmake looks like, but does it also aim the same concise output where output --stat does not fit well, or do folks who daily build with cmake find the output with --stat sit well in the output from other things given there? If the latter, making "the same change" as the Makefile side may not make much sense.
On Tue, Dec 20 2022, Junio C Hamano wrote: > Phillip Wood <phillip.wood123@gmail.com> writes: > >> On 19/12/2022 10:26, Ævar Arnfjörð Bjarmason wrote: >>> In [1] I made the same change to our Makefile, let's follow-up and do >>> the same here. >>> For "cmake" this is particularly nice with "-G Ninja", as before >>> we'd >>> emit ~40 lines of overflowed progress bar output, but now it's only >>> the one line of "ninja"'s progress bar. >> >> I don't really have a strong opinion either way on this but if it >> matches what we do in the Makefile than it sounds sensible. > > As a one-shot change, it might be sensible to claim consistency by > saying "we do the same thing in two places", but I'd worry more > about the root cause of such inconsistency in the first place, i.e. > can we have some trick to ensure that two build systems will not > reimplement the same thing slightly differently? We could & should, but I think doing prep changes like these first makes sense, as eventually e.g. driving both the Makefile & CMake via some shared resource won't have to waste time on explaining why the msgfmt invocation is slightly different. > It also is worth examining if having "the same change" is a good > idea in the first place. The justification given "In [1]" was that > a build driven by our Makefile were concise and non-verbose overall, > but with --stat that concise output pattern was broken. > > I do not know (and I do not have particular interest in knowing) how > a build driven by cmake looks like, but does it also aim the same > concise output where output --stat does not fit well, ... It's the same with CMake, as e.g. the reference to "ninja" in the commit message covers (it would also happen with the "make" backend, but that one's a bit more verbose by default). > [...] or do folks who daily build with cmake find the output with > --stat sit well in the output from other things given there? If the > latter, making "the same change" as the Makefile side may not make > much sense. I think the only justification that's needed here (and which should short-circuit any questions about what someone using cmake may or may not like) is the one given in my 2f12b31b746 (Makefile: don't invoke msgfmt with --statistics, 2021-12-17). I.e. this was something I added as part of the initial i18n support, but I had no good reason for using --statistics other than ad-hoc eyeballing the output at the time. The CMake recipe then just copy/pasted whatever it found in the Makefile, and the two then drifted apart. So, in general with those sorts of changes I think it's sufficient to say that we're not bringing them in line again, unless there's some reason to suppose that the cmake version has since come to rely on the divergence for some reason. Which, in this case is clearly not the case, as we're just spewing this output to the user's terminal.
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > I think the only justification that's needed here (and which should > short-circuit any questions about what someone using cmake may or may > not like) is the one given in my 2f12b31b746 (Makefile: don't invoke > msgfmt with --statistics, 2021-12-17). > > I.e. this was something I added as part of the initial i18n support, but > I had no good reason for using --statistics other than ad-hoc eyeballing > the output at the time. > > The CMake recipe then just copy/pasted whatever it found in the > Makefile, and the two then drifted apart. > > So, in general with those sorts of changes I think it's sufficient to > say that we're not bringing them in line again, unless there's some > reason to suppose that the cmake version has since come to rely on the > divergence for some reason. > > Which, in this case is clearly not the case, as we're just spewing this > output to the user's terminal. OK, let's hear from Windows folks about that. I do not care either way myself (after all it is just extra lines in the output), but for a topic that was once merged to 'next' that later turned out to be unwanted (instead of simply being a buggy implementation of the right idea), I'd like to hear from those who have been depending on whatever the current behaviour is. Thanks.
diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt index 2f6e0197ffa..8f8b6f375f7 100644 --- a/contrib/buildsystems/CMakeLists.txt +++ b/contrib/buildsystems/CMakeLists.txt @@ -897,7 +897,7 @@ if(MSGFMT_EXE) foreach(po ${po_files}) file(MAKE_DIRECTORY ${CMAKE_BINARY_DIR}/po/build/locale/${po}/LC_MESSAGES) add_custom_command(OUTPUT ${CMAKE_BINARY_DIR}/po/build/locale/${po}/LC_MESSAGES/git.mo - COMMAND ${MSGFMT_EXE} --check --statistics -o ${CMAKE_BINARY_DIR}/po/build/locale/${po}/LC_MESSAGES/git.mo ${CMAKE_SOURCE_DIR}/po/${po}.po) + COMMAND ${MSGFMT_EXE} --check -o ${CMAKE_BINARY_DIR}/po/build/locale/${po}/LC_MESSAGES/git.mo ${CMAKE_SOURCE_DIR}/po/${po}.po) list(APPEND po_gen ${CMAKE_BINARY_DIR}/po/build/locale/${po}/LC_MESSAGES/git.mo) endforeach() add_custom_target(po-gen ALL DEPENDS ${po_gen})
In [1] I made the same change to our Makefile, let's follow-up and do the same here. For "cmake" this is particularly nice with "-G Ninja", as before we'd emit ~40 lines of overflowed progress bar output, but now it's only the one line of "ninja"'s progress bar. 1. 2f12b31b746 (Makefile: don't invoke msgfmt with --statistics, 2021-12-17) Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- This trivial fix is extracted from the ab/cmake-nix-and-ci topic which was ejected around the time of the release for that previous submission see [1], the range-diff is to that topic. I'm re-arranging and re-submitting topic more piecemeal. There were no outstanding issues or feedback with this part of it, so hopefully this can advance relatively quickly. I'll also submit some of the other uncontroversial bits today independently, none of which conflict with one another. Then once those have landed try to find some acceptable way forward for the later bits, which at that point will be easier to review. 1. https://lore.kernel.org/git/cover-v6-00.15-00000000000-20221206T001617Z-avarab@gmail.com/ Range-diff: 1: fc190b379cd = 1: 0fa41115261 cmake: don't invoke msgfmt with --statistics 2: 1a11aa233a3 < -: ----------- cmake: use "-S" and "-B" to specify source and build directories 3: b9ddb5db1d3 < -: ----------- cmake: update instructions for portable CMakeLists.txt 4: 7b7850c00ee < -: ----------- cmake: don't copy chainlint.pl to build directory 5: 82ecb797915 < -: ----------- cmake: chmod +x the bin-wrappers/* & SCRIPT_{SH,PERL} & git-p4 6: 1f326944a07 < -: ----------- cmake & test-lib.sh: add a $GIT_SOURCE_DIR variable 7: 973c8038f54 < -: ----------- cmake: set "USE_LIBPCRE2" in "GIT-BUILD-OPTIONS" for test-lib.sh 8: b8448c7a8a6 < -: ----------- Makefile + test-lib.sh: don't prefer cmake-built to make-built git 9: 5135e40969e < -: ----------- test-lib.sh: support a "GIT_TEST_BUILD_DIR" 10: 65204463730 < -: ----------- cmake: optionally be able to run tests before "ctest" 11: e25992b16f1 < -: ----------- cmake: support GIT_TEST_OPTS, abstract away WIN32 defaults 12: 4905ce5321d < -: ----------- cmake: increase test timeout on Windows only 13: 6c6b530965d < -: ----------- cmake: only look for "sh" in "C:/Program Files" on Windows 14: 563f1b9b045 < -: ----------- cmake: copy over git-p4.py for t983[56] perforce test 15: 917a884eb65 < -: ----------- CI: add a "linux-cmake-test" to run cmake & ctest on linux contrib/buildsystems/CMakeLists.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)