ci: make sure we build Git parallel
diff mbox series

Message ID 20190207183736.9299-1-szeder.dev@gmail.com
State New
Headers show
Series
  • ci: make sure we build Git parallel
Related show

Commit Message

SZEDER Gábor Feb. 7, 2019, 6:37 p.m. UTC
Commit 2c8921db2b (travis-ci: build with the right compiler,
2019-01-17) started to use MAKEFLAGS to specify which compiler to use
to build Git.  A bit later, and in a different topic branch commit
eaa62291ff (ci: inherit --jobs via MAKEFLAGS in run-build-and-tests,
2019-01-27) started to use MAKEFLAGS as well.  Unfortunately, there is
a semantic conflict between these two commits: both of them set
MAKEFLAGS, and since the line adding CC from 2c8921db2b comes later in
'ci/lib.sh', it overwrites the number of parallel jobs added in
eaa62291ff.

Consequently, since both commits have been merged all our CI jobs have
been building Git, building its documentation, and applying semantic
patches sequentially, making all build jobs a bit slower.  Running
the test suite is unaffected, because the number of test jobs comes
from GIT_PROVE_OPTS.

Append to MAKEFLAGS when setting the compiler to use, to ensure that
the number of parallel jobs to use is preserved.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 ci/lib.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Junio C Hamano Feb. 7, 2019, 7 p.m. UTC | #1
SZEDER Gábor <szeder.dev@gmail.com> writes:

> Append to MAKEFLAGS when setting the compiler to use, to ensure that
> the number of parallel jobs to use is preserved.
>
> Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
> ---
>  ci/lib.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/ci/lib.sh b/ci/lib.sh
> index 16f4ecbc67..cee51a4cc4 100755
> --- a/ci/lib.sh
> +++ b/ci/lib.sh
> @@ -185,4 +185,4 @@ GIT_TEST_GETTEXT_POISON)
>  	;;
>  esac
>  
> -export MAKEFLAGS="CC=${CC:-cc}"
> +export MAKEFLAGS="$MAKEFLAGS CC=${CC:-cc}"

Makes sense.  Thanks.
Junio C Hamano Feb. 7, 2019, 7:04 p.m. UTC | #2
Junio C Hamano <gitster@pobox.com> writes:

> SZEDER Gábor <szeder.dev@gmail.com> writes:
>
>> Append to MAKEFLAGS when setting the compiler to use, to ensure that
>> the number of parallel jobs to use is preserved.
>>
>> Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
>> ---
>>  ci/lib.sh | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/ci/lib.sh b/ci/lib.sh
>> index 16f4ecbc67..cee51a4cc4 100755
>> --- a/ci/lib.sh
>> +++ b/ci/lib.sh
>> @@ -185,4 +185,4 @@ GIT_TEST_GETTEXT_POISON)
>>  	;;
>>  esac
>>  
>> -export MAKEFLAGS="CC=${CC:-cc}"
>> +export MAKEFLAGS="$MAKEFLAGS CC=${CC:-cc}"
>
> Makes sense.  Thanks.

Wouldn't all other hits of "MAKEFLAGS=" in ci/lib.sh also want the
same treatment, though?  We know that "if travis to this, otherwise
if Asure, do that" is the first block to muck with MAKEFLAGS in the
script, but a new assignment before that block can be added in the
future and cause a similar issue unless we do so.

Of course, at some point we do want to say "we do not want to
inherit it from the outside environment", but then such an
assignment of empty value should be done at the very beginning with
a comment, not with "this happens to be the first one to set, so
let's not append but assign to clear any previous value", I would
think.
Junio C Hamano Feb. 7, 2019, 7:35 p.m. UTC | #3
Junio C Hamano <gitster@pobox.com> writes:

> Wouldn't all other hits of "MAKEFLAGS=" in ci/lib.sh also want the
> same treatment, though?  We know that "if travis to this, otherwise
> if Asure, do that" is the first block to muck with MAKEFLAGS in the
> script, but a new assignment before that block can be added in the
> future and cause a similar issue unless we do so.
>
> Of course, at some point we do want to say "we do not want to
> inherit it from the outside environment", but then such an
> assignment of empty value should be done at the very beginning with
> a comment, not with "this happens to be the first one to set, so
> let's not append but assign to clear any previous value", I would
> think.

That is, in a patch form on top of yours, something like this.


 ci/lib.sh | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/ci/lib.sh b/ci/lib.sh
index cee51a4cc4..288a5b3884 100755
--- a/ci/lib.sh
+++ b/ci/lib.sh
@@ -74,6 +74,9 @@ check_unignored_build_artifacts ()
 	}
 }
 
+# Clear MAKEFLAGS that may come from the outside world.
+export MAKEFLAGS=
+
 # Set 'exit on error' for all CI scripts to let the caller know that
 # something went wrong.
 # Set tracing executed commands, primarily setting environment variables
@@ -101,7 +104,7 @@ then
 	BREW_INSTALL_PACKAGES="git-lfs gettext"
 	export GIT_PROVE_OPTS="--timer --jobs 3 --state=failed,slow,save"
 	export GIT_TEST_OPTS="--verbose-log -x --immediate"
-	export MAKEFLAGS="--jobs=2"
+	MAKEFLAGS="$MAKEFLAGS --jobs=2"
 elif test -n "$SYSTEM_COLLECTIONURI" || test -n "$SYSTEM_TASKDEFINITIONSURI"
 then
 	CI_TYPE=azure-pipelines
@@ -126,7 +129,7 @@ then
 	BREW_INSTALL_PACKAGES=gcc@8
 	export GIT_PROVE_OPTS="--timer --jobs 10 --state=failed,slow,save"
 	export GIT_TEST_OPTS="--verbose-log -x --write-junit-xml"
-	export MAKEFLAGS="--jobs=10"
+	MAKEFLAGS="$MAKEFLAGS --jobs=10"
 	test windows_nt != "$CI_OS_NAME" ||
 	GIT_TEST_OPTS="--no-chain-lint --no-bin-wrappers $GIT_TEST_OPTS"
 else
@@ -185,4 +188,4 @@ GIT_TEST_GETTEXT_POISON)
 	;;
 esac
 
-export MAKEFLAGS="$MAKEFLAGS CC=${CC:-cc}"
+MAKEFLAGS="$MAKEFLAGS CC=${CC:-cc}"
Johannes Schindelin Feb. 7, 2019, 10:32 p.m. UTC | #4
Hi Junio,

On Thu, 7 Feb 2019, Junio C Hamano wrote:

> Junio C Hamano <gitster@pobox.com> writes:
> 
> > Wouldn't all other hits of "MAKEFLAGS=" in ci/lib.sh also want the
> > same treatment, though?  We know that "if travis to this, otherwise
> > if Asure, do that" is the first block to muck with MAKEFLAGS in the
> > script, but a new assignment before that block can be added in the
> > future and cause a similar issue unless we do so.
> >
> > Of course, at some point we do want to say "we do not want to
> > inherit it from the outside environment", but then such an
> > assignment of empty value should be done at the very beginning with
> > a comment, not with "this happens to be the first one to set, so
> > let's not append but assign to clear any previous value", I would
> > think.
> 
> That is, in a patch form on top of yours, something like this.
> 
> 
>  ci/lib.sh | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/ci/lib.sh b/ci/lib.sh
> index cee51a4cc4..288a5b3884 100755
> --- a/ci/lib.sh
> +++ b/ci/lib.sh
> @@ -74,6 +74,9 @@ check_unignored_build_artifacts ()
>  	}
>  }
>  
> +# Clear MAKEFLAGS that may come from the outside world.
> +export MAKEFLAGS=
> +
>  # Set 'exit on error' for all CI scripts to let the caller know that
>  # something went wrong.
>  # Set tracing executed commands, primarily setting environment variables
> @@ -101,7 +104,7 @@ then
>  	BREW_INSTALL_PACKAGES="git-lfs gettext"
>  	export GIT_PROVE_OPTS="--timer --jobs 3 --state=failed,slow,save"
>  	export GIT_TEST_OPTS="--verbose-log -x --immediate"
> -	export MAKEFLAGS="--jobs=2"
> +	MAKEFLAGS="$MAKEFLAGS --jobs=2"
>  elif test -n "$SYSTEM_COLLECTIONURI" || test -n "$SYSTEM_TASKDEFINITIONSURI"
>  then
>  	CI_TYPE=azure-pipelines
> @@ -126,7 +129,7 @@ then
>  	BREW_INSTALL_PACKAGES=gcc@8
>  	export GIT_PROVE_OPTS="--timer --jobs 10 --state=failed,slow,save"
>  	export GIT_TEST_OPTS="--verbose-log -x --write-junit-xml"
> -	export MAKEFLAGS="--jobs=10"
> +	MAKEFLAGS="$MAKEFLAGS --jobs=10"
>  	test windows_nt != "$CI_OS_NAME" ||
>  	GIT_TEST_OPTS="--no-chain-lint --no-bin-wrappers $GIT_TEST_OPTS"
>  else
> @@ -185,4 +188,4 @@ GIT_TEST_GETTEXT_POISON)
>  	;;
>  esac
>  
> -export MAKEFLAGS="$MAKEFLAGS CC=${CC:-cc}"
> +MAKEFLAGS="$MAKEFLAGS CC=${CC:-cc}"

Since this is intended to be run in a CI setting, there is not a whole lot
of opportunity to set `MAKEFLAGS` outside of the script. And if there is,
that might open a rabbit hole when debugging issues that somehow in the
end turn out to come from a hard-coded `MAKEFLAGS` somewhere in the CI
system.

So I'd rather clear `MAKEFLAGS` at the beginning (i.e. where you `export
MAKEFLAGS`, I'd simply append a `=`).

Ciao,
Dscho
Junio C Hamano Feb. 7, 2019, 11:33 p.m. UTC | #5
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

>>  
>> +# Clear MAKEFLAGS that may come from the outside world.
>> +export MAKEFLAGS=
>> +
>>  # Set 'exit on error' for all CI scripts to let the caller know that
>>  # something went wrong.
>>  # Set tracing executed commands, primarily setting environment variables
>> @@ -101,7 +104,7 @@ then
>>  	BREW_INSTALL_PACKAGES="git-lfs gettext"
>>  	export GIT_PROVE_OPTS="--timer --jobs 3 --state=failed,slow,save"
>>  	export GIT_TEST_OPTS="--verbose-log -x --immediate"
>> -	export MAKEFLAGS="--jobs=2"
>> +	MAKEFLAGS="$MAKEFLAGS --jobs=2"
>>  elif test -n "$SYSTEM_COLLECTIONURI" || test -n "$SYSTEM_TASKDEFINITIONSURI"
>>  then
>>  	CI_TYPE=azure-pipelines
>> @@ -126,7 +129,7 @@ then
>>  	BREW_INSTALL_PACKAGES=gcc@8
>>  	export GIT_PROVE_OPTS="--timer --jobs 10 --state=failed,slow,save"
>>  	export GIT_TEST_OPTS="--verbose-log -x --write-junit-xml"
>> -	export MAKEFLAGS="--jobs=10"
>> +	MAKEFLAGS="$MAKEFLAGS --jobs=10"
>>  	test windows_nt != "$CI_OS_NAME" ||
>>  	GIT_TEST_OPTS="--no-chain-lint --no-bin-wrappers $GIT_TEST_OPTS"
>>  else
>> @@ -185,4 +188,4 @@ GIT_TEST_GETTEXT_POISON)
>>  	;;
>>  esac
>>  
>> -export MAKEFLAGS="$MAKEFLAGS CC=${CC:-cc}"
>> +MAKEFLAGS="$MAKEFLAGS CC=${CC:-cc}"
>
> Since this is intended to be run in a CI setting, there is not a whole lot
> of opportunity to set `MAKEFLAGS` outside of the script. And if there is,
> that might open a rabbit hole when debugging issues that somehow in the
> end turn out to come from a hard-coded `MAKEFLAGS` somewhere in the CI
> system.
>
> So I'd rather clear `MAKEFLAGS` at the beginning (i.e. where you `export
> MAKEFLAGS`, I'd simply append a `=`).

I meant to clear it at the beginning, where I "export MAKEFLAGS=".
Did your MUA ate the equal sign at the end, mistaking it with part
of text/plain; format=flawed or something?
Johannes Schindelin Feb. 8, 2019, 10:10 a.m. UTC | #6
Hi Junio,

On Thu, 7 Feb 2019, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> >>  
> >> +# Clear MAKEFLAGS that may come from the outside world.
> >> +export MAKEFLAGS=
> >> +
> >>  # Set 'exit on error' for all CI scripts to let the caller know that
> >>  # something went wrong.
> >>  # Set tracing executed commands, primarily setting environment variables
> >> @@ -101,7 +104,7 @@ then
> >>  	BREW_INSTALL_PACKAGES="git-lfs gettext"
> >>  	export GIT_PROVE_OPTS="--timer --jobs 3 --state=failed,slow,save"
> >>  	export GIT_TEST_OPTS="--verbose-log -x --immediate"
> >> -	export MAKEFLAGS="--jobs=2"
> >> +	MAKEFLAGS="$MAKEFLAGS --jobs=2"
> >>  elif test -n "$SYSTEM_COLLECTIONURI" || test -n "$SYSTEM_TASKDEFINITIONSURI"
> >>  then
> >>  	CI_TYPE=azure-pipelines
> >> @@ -126,7 +129,7 @@ then
> >>  	BREW_INSTALL_PACKAGES=gcc@8
> >>  	export GIT_PROVE_OPTS="--timer --jobs 10 --state=failed,slow,save"
> >>  	export GIT_TEST_OPTS="--verbose-log -x --write-junit-xml"
> >> -	export MAKEFLAGS="--jobs=10"
> >> +	MAKEFLAGS="$MAKEFLAGS --jobs=10"
> >>  	test windows_nt != "$CI_OS_NAME" ||
> >>  	GIT_TEST_OPTS="--no-chain-lint --no-bin-wrappers $GIT_TEST_OPTS"
> >>  else
> >> @@ -185,4 +188,4 @@ GIT_TEST_GETTEXT_POISON)
> >>  	;;
> >>  esac
> >>  
> >> -export MAKEFLAGS="$MAKEFLAGS CC=${CC:-cc}"
> >> +MAKEFLAGS="$MAKEFLAGS CC=${CC:-cc}"
> >
> > Since this is intended to be run in a CI setting, there is not a whole lot
> > of opportunity to set `MAKEFLAGS` outside of the script. And if there is,
> > that might open a rabbit hole when debugging issues that somehow in the
> > end turn out to come from a hard-coded `MAKEFLAGS` somewhere in the CI
> > system.
> >
> > So I'd rather clear `MAKEFLAGS` at the beginning (i.e. where you `export
> > MAKEFLAGS`, I'd simply append a `=`).
> 
> I meant to clear it at the beginning, where I "export MAKEFLAGS=".
> Did your MUA ate the equal sign at the end, mistaking it with part
> of text/plain; format=flawed or something?

I could have sworn that there was no equal sign yesterday.

Sorry for the noise,
Dscho
Junio C Hamano Feb. 8, 2019, 5:25 p.m. UTC | #7
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> Hi Junio,
>
> On Thu, 7 Feb 2019, Junio C Hamano wrote:
>
>> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>> 
>> >>  
>> >> +# Clear MAKEFLAGS that may come from the outside world.
>> >> +export MAKEFLAGS=
>> >> +
>> ...
>> I meant to clear it at the beginning, where I "export MAKEFLAGS=".
>> Did your MUA ate the equal sign at the end, mistaking it with part
>> of text/plain; format=flawed or something?
>
> I could have sworn that there was no equal sign yesterday.
>
> Sorry for the noise,

Don't be.  Mistakes happen and watching stupid mistakes for each
other is part of the teamwork I very much appreciate.

Will apply on top of Szeder's fix.  Thanks.

Patch
diff mbox series

diff --git a/ci/lib.sh b/ci/lib.sh
index 16f4ecbc67..cee51a4cc4 100755
--- a/ci/lib.sh
+++ b/ci/lib.sh
@@ -185,4 +185,4 @@  GIT_TEST_GETTEXT_POISON)
 	;;
 esac
 
-export MAKEFLAGS="CC=${CC:-cc}"
+export MAKEFLAGS="$MAKEFLAGS CC=${CC:-cc}"