[PATCH] cmake: don't invoke msgfmt with --statistics

[PATCH] cmake: don't invoke msgfmt with --statistics

[Ævar Arnfjörð Bjarmason]

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(-)

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})
-- 
2.39.0.1071.g97ce8966538

Re: [PATCH] cmake: don't invoke msgfmt with --statistics

[Phillip Wood]

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})

Re: [PATCH] cmake: don't invoke msgfmt with --statistics

[Junio C Hamano]

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.

Re: [PATCH] cmake: don't invoke msgfmt with --statistics

[Ævar Arnfjörð Bjarmason]

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.

Re: [PATCH] cmake: don't invoke msgfmt with --statistics

[Junio C Hamano]

Æ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.