diff mbox series

[1/6] ci: remove GETTEXT_POISON jobs

Message ID 20210111144740.6092-2-avarab@gmail.com (mailing list archive)
State New, archived
Headers show
Series [1/6] ci: remove GETTEXT_POISON jobs | expand

Commit Message

Ævar Arnfjörð Bjarmason Jan. 11, 2021, 2:47 p.m. UTC
A subsequent commit will remove GETTEXT_POISON entirely, let's start
by removing the CI jobs that enable the option.

We cannot just remove the job because the CI is implicitly depending
on the "poison" job being a sort of "default" job. Let's instead add a
"default" job.

This means we can remove the initial "make test" from the "linux-gcc"
job (it does another one after setting a bunch of GIT_TEST_*
variables).

I'm not doing that because it would conflict with the in-flight
334afbc76fb (tests: mark tests relying on the current default for
`init.defaultBranch`, 2020-11-18) (currently on the "seen" branch, so
the SHA-1 will almost definitely change). It's going to use that "make
test" again for different reasons, so let's preserve it for now.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 .github/workflows/main.yml | 2 +-
 .travis.yml                | 2 +-
 ci/install-dependencies.sh | 2 +-
 ci/lib.sh                  | 3 +--
 4 files changed, 4 insertions(+), 5 deletions(-)

Comments

SZEDER Gábor Jan. 12, 2021, 8:50 a.m. UTC | #1
On Mon, Jan 11, 2021 at 03:47:35PM +0100, Ævar Arnfjörð Bjarmason wrote:
> A subsequent commit will remove GETTEXT_POISON entirely, let's start
> by removing the CI jobs that enable the option.
> 
> We cannot just remove the job because the CI is implicitly depending
> on the "poison" job being a sort of "default" job.

I don't understand what you mean here with a "default job" that the CI
is implicitly depending on.  There is certainly no such default job on
Travis CI, and I don't think there is one on the GitHub thing.

> Let's instead add a
> "default" job.
> 
> This means we can remove the initial "make test" from the "linux-gcc"
> job (it does another one after setting a bunch of GIT_TEST_*
> variables).
> 
> I'm not doing that because it would conflict with the in-flight
> 334afbc76fb (tests: mark tests relying on the current default for
> `init.defaultBranch`, 2020-11-18) (currently on the "seen" branch, so
> the SHA-1 will almost definitely change). It's going to use that "make
> test" again for different reasons, so let's preserve it for now.
> 
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  .github/workflows/main.yml | 2 +-
>  .travis.yml                | 2 +-
>  ci/install-dependencies.sh | 2 +-
>  ci/lib.sh                  | 3 +--
>  4 files changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml
> index aef6643648..8b52df200f 100644
> --- a/.github/workflows/main.yml
> +++ b/.github/workflows/main.yml
> @@ -286,7 +286,7 @@ jobs:
>            - jobname: osx-gcc
>              cc: gcc
>              pool: macos-latest
> -          - jobname: GETTEXT_POISON
> +          - jobname: linux-gcc-default
>              cc: gcc
>              pool: ubuntu-latest
>      env:
> diff --git a/.travis.yml b/.travis.yml
> index 05f3e3f8d7..908330a0a3 100644
> --- a/.travis.yml
> +++ b/.travis.yml
> @@ -16,7 +16,7 @@ compiler:
>  
>  matrix:
>    include:
> -    - env: jobname=GETTEXT_POISON
> +    - env: jobname=linux-gcc-default
>        os: linux
>        compiler:
>        addons:
> diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh
> index 0229a77f7d..79c0633a18 100755
> --- a/ci/install-dependencies.sh
> +++ b/ci/install-dependencies.sh
> @@ -72,7 +72,7 @@ Documentation)
>  	test -n "$ALREADY_HAVE_ASCIIDOCTOR" ||
>  	sudo gem install --version 1.5.8 asciidoctor
>  	;;
> -linux-gcc-4.8|GETTEXT_POISON)
> +linux-gcc-default|linux-gcc-4.8)
>  	sudo apt-get -q update
>  	sudo apt-get -q -y install $UBUNTU_COMMON_PKGS
>  	;;
> diff --git a/ci/lib.sh b/ci/lib.sh
> index 38c0eac351..d848c036c5 100755
> --- a/ci/lib.sh
> +++ b/ci/lib.sh
> @@ -220,8 +220,7 @@ osx-clang|osx-gcc)
>  	# Travis CI OS X
>  	export GIT_SKIP_TESTS="t9810 t9816"
>  	;;
> -GETTEXT_POISON)
> -	export GIT_TEST_GETTEXT_POISON=true
> +linux-gcc-default)
>  	;;
>  Linux32)
>  	CC=gcc
> -- 
> 2.29.2.222.g5d2a92d10f8
>
Ævar Arnfjörð Bjarmason Jan. 20, 2021, 5:59 p.m. UTC | #2
On Tue, Jan 12 2021, SZEDER Gábor wrote:

> On Mon, Jan 11, 2021 at 03:47:35PM +0100, Ævar Arnfjörð Bjarmason wrote:
>> A subsequent commit will remove GETTEXT_POISON entirely, let's start
>> by removing the CI jobs that enable the option.
>> 
>> We cannot just remove the job because the CI is implicitly depending
>> on the "poison" job being a sort of "default" job.
>
> I don't understand what you mean here with a "default job" that the CI
> is implicitly depending on.  There is certainly no such default job on
> Travis CI, and I don't think there is one on the GitHub thing.

I'll reword this. I meant that the poison job was using the default
compiler etc., whereas the other ones were setting custom values.

I vaguely remember some list traffic about this in the past, i.e. the
reliance on the poison job not just for that, but also as "the default
OS image" setup.
SZEDER Gábor Jan. 20, 2021, 7:14 p.m. UTC | #3
On Wed, Jan 20, 2021 at 06:59:14PM +0100, Ævar Arnfjörð Bjarmason wrote:
> 
> On Tue, Jan 12 2021, SZEDER Gábor wrote:
> 
> > On Mon, Jan 11, 2021 at 03:47:35PM +0100, Ævar Arnfjörð Bjarmason wrote:
> >> A subsequent commit will remove GETTEXT_POISON entirely, let's start
> >> by removing the CI jobs that enable the option.
> >> 
> >> We cannot just remove the job because the CI is implicitly depending
> >> on the "poison" job being a sort of "default" job.
> >
> > I don't understand what you mean here with a "default job" that the CI
> > is implicitly depending on.  There is certainly no such default job on
> > Travis CI, and I don't think there is one on the GitHub thing.
> 
> I'll reword this. I meant that the poison job was using the default
> compiler etc., whereas the other ones were setting custom values.
> 
> I vaguely remember some list traffic about this in the past, i.e. the
> reliance on the poison job not just for that, but also as "the default
> OS image" setup.

Oh, I didn't mean that the commit message should be clarified.  I
meant that the GETTEXT_POISON job can simply be deleted.  Sorry for
not being clear enough.
Ævar Arnfjörð Bjarmason Jan. 27, 2021, 12:47 a.m. UTC | #4
On Wed, Jan 20 2021, SZEDER Gábor wrote:

> On Wed, Jan 20, 2021 at 06:59:14PM +0100, Ævar Arnfjörð Bjarmason wrote:
>> 
>> On Tue, Jan 12 2021, SZEDER Gábor wrote:
>> 
>> > On Mon, Jan 11, 2021 at 03:47:35PM +0100, Ævar Arnfjörð Bjarmason wrote:
>> >> A subsequent commit will remove GETTEXT_POISON entirely, let's start
>> >> by removing the CI jobs that enable the option.
>> >> 
>> >> We cannot just remove the job because the CI is implicitly depending
>> >> on the "poison" job being a sort of "default" job.
>> >
>> > I don't understand what you mean here with a "default job" that the CI
>> > is implicitly depending on.  There is certainly no such default job on
>> > Travis CI, and I don't think there is one on the GitHub thing.
>> 
>> I'll reword this. I meant that the poison job was using the default
>> compiler etc., whereas the other ones were setting custom values.
>> 
>> I vaguely remember some list traffic about this in the past, i.e. the
>> reliance on the poison job not just for that, but also as "the default
>> OS image" setup.
>
> Oh, I didn't mean that the commit message should be clarified.  I
> meant that the GETTEXT_POISON job can simply be deleted.  Sorry for
> not being clear enough.

I still don't get it, and my patch still seems fine as it is to me. But
I'm probably missing something.

I'm looking at these, both pointing at e6362826a04 (The fourth batch,
2021-01-25):

    # Currently says "Please be aware travis-ci.org will be shutting
    # down in several weeks"
    https://www.travis-ci.org/github/git/git/builds/756176098

    https://github.com/git/git/actions/runs/510676142

For the GitHub one, there's only two things that are linux && gcc. The
linux-gcc job, and GETTEXT_POISON (renamed to linux-gcc-default in this
series).

In ci/install-dependencies.sh we install GCC 8 for linux-gcc, but also
perforce, apache, git-lfs etc.

Then in ci/lib.sh we set CC=gcc-8 on linux-gcc, but retain the default
on GETTEXT_POISON. We also set GIT_TEST_HTTPD=true.

So hence "linux-gcc-default", we're running as close to a default set of
packages we can get and build/run git at all. I.e. just "apt install
$UBUNTU_COMMON_PKGS".

Then in ci/run-build-and-tests.sh the "linux-gcc" job is also our "let's
turn on every GIT_TEST_* option you've heard of" job. Whereas on the
GETTEXT_POISON job we just ran "make test", well, with
GIT_TEST_GETTEXT_POISON=true before this series, but that was the only
non-default, now we've got no non-defaults.

I can see why we'd want this on top:
    
    diff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh
    index 50e0b90073f..3071099eaca 100755
    --- a/ci/run-build-and-tests.sh
    +++ b/ci/run-build-and-tests.sh
    @@ -32,11 +32,6 @@ linux-clang)
            export GIT_TEST_DEFAULT_HASH=sha256
            make test
            ;;
    -linux-gcc-4.8)
    -       # Don't run the tests; we only care about whether Git can be
    -       # built with GCC 4.8, as it errors out on some undesired (C99)
    -       # constructs that newer compilers seem to quietly accept.
    -       ;;
     *)
            make test
            ;;

Because there we're skipping tests on linux-gcc-4.8, but on travis it's
the only Ubuntu Trusty image (default is Xenial), so yes we build with
gcc 4.8, but we're also missing out in seeing if our tests are working
on a slightly older Ubuntu.

Also on GitHub's page the GETTEXT_POISON job finishes in ~10m but all
the other ones in ~30m, I see some run "make test" twice, as an aside is
there some limit on jobs but not overall CPU? Otherwise those would
finish faster if that was split up.

But "make test" twice doesn't account for a ~3x increase, so it's got to
be the extra tests we're running, i.e. svn tests, httpd etc.

It seems to me to provide value in itself to run some shorter version of
the CI for people using it as a poor man's edit/push/ci/edit loop.
SZEDER Gábor Feb. 1, 2021, 10:04 p.m. UTC | #5
On Wed, Jan 27, 2021 at 01:47:27AM +0100, Ævar Arnfjörð Bjarmason wrote:
> 
> On Wed, Jan 20 2021, SZEDER Gábor wrote:
> 
> > On Wed, Jan 20, 2021 at 06:59:14PM +0100, Ævar Arnfjörð Bjarmason wrote:
> >> 
> >> On Tue, Jan 12 2021, SZEDER Gábor wrote:
> >> 
> >> > On Mon, Jan 11, 2021 at 03:47:35PM +0100, Ævar Arnfjörð Bjarmason wrote:
> >> >> A subsequent commit will remove GETTEXT_POISON entirely, let's start
> >> >> by removing the CI jobs that enable the option.
> >> >> 
> >> >> We cannot just remove the job because the CI is implicitly depending
> >> >> on the "poison" job being a sort of "default" job.
> >> >
> >> > I don't understand what you mean here with a "default job" that the CI
> >> > is implicitly depending on.  There is certainly no such default job on
> >> > Travis CI, and I don't think there is one on the GitHub thing.
> >> 
> >> I'll reword this. I meant that the poison job was using the default
> >> compiler etc., whereas the other ones were setting custom values.
> >> 
> >> I vaguely remember some list traffic about this in the past, i.e. the
> >> reliance on the poison job not just for that, but also as "the default
> >> OS image" setup.
> >
> > Oh, I didn't mean that the commit message should be clarified.  I
> > meant that the GETTEXT_POISON job can simply be deleted.  Sorry for
> > not being clear enough.
> 
> I still don't get it, and my patch still seems fine as it is to me. But
> I'm probably missing something.
> 
> I'm looking at these, both pointing at e6362826a04 (The fourth batch,
> 2021-01-25):
> 
>     # Currently says "Please be aware travis-ci.org will be shutting
>     # down in several weeks"

... and continues with:

  "with all accounts migrating to travis-ci.com.  Please stay tuned
   here for more information."

FWIW, I recently migrated manually, and apart from the different TLD
haven't noticed anything different (though somehow the jobs in my .org
builds sometimes took a while to start lately, but on .com they all
started real fast so far, as they should).

>     https://www.travis-ci.org/github/git/git/builds/756176098
> 
>     https://github.com/git/git/actions/runs/510676142
> 
> For the GitHub one, there's only two things that are linux && gcc. The
> linux-gcc job, and GETTEXT_POISON (renamed to linux-gcc-default in this
> series).
> 
> In ci/install-dependencies.sh we install GCC 8 for linux-gcc, but also
> perforce, apache, git-lfs etc.
> 
> Then in ci/lib.sh we set CC=gcc-8 on linux-gcc, but retain the default
> on GETTEXT_POISON. We also set GIT_TEST_HTTPD=true.
> 
> So hence "linux-gcc-default", we're running as close to a default set of
> packages we can get and build/run git at all. I.e. just "apt install
> $UBUNTU_COMMON_PKGS".

We started installing newer compilers, because we wanted to benefit
from their better error reporting and improved warnings.  The reason
we did that in existing CI jobs without adding a "default" job was
that we don't care enough about the default compiler and configuration
to justify its additional runtime, because we run the test suite
enough times already.

Considering that the number of CI jobs has grown since then, I would
rather see the build complete faster than a default job running the
test suite for the N+1th time.

> Then in ci/run-build-and-tests.sh the "linux-gcc" job is also our "let's
> turn on every GIT_TEST_* option you've heard of" job. Whereas on the
> GETTEXT_POISON job we just ran "make test", well, with
> GIT_TEST_GETTEXT_POISON=true before this series, but that was the only
> non-default, now we've got no non-defaults.
> 
> I can see why we'd want this on top:
>     
>     diff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh
>     index 50e0b90073f..3071099eaca 100755
>     --- a/ci/run-build-and-tests.sh
>     +++ b/ci/run-build-and-tests.sh
>     @@ -32,11 +32,6 @@ linux-clang)
>             export GIT_TEST_DEFAULT_HASH=sha256
>             make test
>             ;;
>     -linux-gcc-4.8)
>     -       # Don't run the tests; we only care about whether Git can be
>     -       # built with GCC 4.8, as it errors out on some undesired (C99)
>     -       # constructs that newer compilers seem to quietly accept.
>     -       ;;
>      *)
>             make test
>             ;;
> 
> Because there we're skipping tests on linux-gcc-4.8, but on travis it's
> the only Ubuntu Trusty image (default is Xenial), so yes we build with
> gcc 4.8, but we're also missing out in seeing if our tests are working
> on a slightly older Ubuntu.

The above comment you propose to remove explains why you should not
remove it...  And, again, running the test suite doesn't seem to worth
the extra runtime.


> Also on GitHub's page the GETTEXT_POISON job finishes in ~10m but all
> the other ones in ~30m, I see some run "make test" twice, as an aside is
> there some limit on jobs but not overall CPU? Otherwise those would
> finish faster if that was split up.
> 
> But "make test" twice doesn't account for a ~3x increase, so it's got to
> be the extra tests we're running, i.e. svn tests, httpd etc.
> 
> It seems to me to provide value in itself to run some shorter version of
> the CI for people using it as a poor man's edit/push/ci/edit loop.
Ævar Arnfjörð Bjarmason Feb. 3, 2021, 12:13 p.m. UTC | #6
On Mon, Feb 01 2021, SZEDER Gábor wrote:

> On Wed, Jan 27, 2021 at 01:47:27AM +0100, Ævar Arnfjörð Bjarmason wrote:
>> 
>> On Wed, Jan 20 2021, SZEDER Gábor wrote:
>> 
>> > On Wed, Jan 20, 2021 at 06:59:14PM +0100, Ævar Arnfjörð Bjarmason wrote:
>> >> 
>> >> On Tue, Jan 12 2021, SZEDER Gábor wrote:
>> >> 
>> >> > On Mon, Jan 11, 2021 at 03:47:35PM +0100, Ævar Arnfjörð Bjarmason wrote:
>> >> >> A subsequent commit will remove GETTEXT_POISON entirely, let's start
>> >> >> by removing the CI jobs that enable the option.
>> >> >> 
>> >> >> We cannot just remove the job because the CI is implicitly depending
>> >> >> on the "poison" job being a sort of "default" job.
>> >> >
>> >> > I don't understand what you mean here with a "default job" that the CI
>> >> > is implicitly depending on.  There is certainly no such default job on
>> >> > Travis CI, and I don't think there is one on the GitHub thing.
>> >> 
>> >> I'll reword this. I meant that the poison job was using the default
>> >> compiler etc., whereas the other ones were setting custom values.
>> >> 
>> >> I vaguely remember some list traffic about this in the past, i.e. the
>> >> reliance on the poison job not just for that, but also as "the default
>> >> OS image" setup.
>> >
>> > Oh, I didn't mean that the commit message should be clarified.  I
>> > meant that the GETTEXT_POISON job can simply be deleted.  Sorry for
>> > not being clear enough.
>> 
>> I still don't get it, and my patch still seems fine as it is to me. But
>> I'm probably missing something.
>> 
>> I'm looking at these, both pointing at e6362826a04 (The fourth batch,
>> 2021-01-25):
>> 
>>     # Currently says "Please be aware travis-ci.org will be shutting
>>     # down in several weeks"
>
> ... and continues with:
>
>   "with all accounts migrating to travis-ci.com.  Please stay tuned
>    here for more information."
>
> FWIW, I recently migrated manually, and apart from the different TLD
> haven't noticed anything different (though somehow the jobs in my .org
> builds sometimes took a while to start lately, but on .com they all
> started real fast so far, as they should).
>
>>     https://www.travis-ci.org/github/git/git/builds/756176098
>> 
>>     https://github.com/git/git/actions/runs/510676142
>> 
>> For the GitHub one, there's only two things that are linux && gcc. The
>> linux-gcc job, and GETTEXT_POISON (renamed to linux-gcc-default in this
>> series).
>> 
>> In ci/install-dependencies.sh we install GCC 8 for linux-gcc, but also
>> perforce, apache, git-lfs etc.
>> 
>> Then in ci/lib.sh we set CC=gcc-8 on linux-gcc, but retain the default
>> on GETTEXT_POISON. We also set GIT_TEST_HTTPD=true.
>> 
>> So hence "linux-gcc-default", we're running as close to a default set of
>> packages we can get and build/run git at all. I.e. just "apt install
>> $UBUNTU_COMMON_PKGS".
>
> We started installing newer compilers, because we wanted to benefit
> from their better error reporting and improved warnings.  The reason
> we did that in existing CI jobs without adding a "default" job was
> that we don't care enough about the default compiler and configuration
> to justify its additional runtime, because we run the test suite
> enough times already.

So in summary, you'd like it removed because the combination of
GETTEXT_POISON + old compiler is no longer worth it, but while we were
running with GETTEXT_POISON anyway we might as well run it on the old
compiler?

> Considering that the number of CI jobs has grown since then, I would
> rather see the build complete faster than a default job running the
> test suite for the N+1th time.

[...]

>> Then in ci/run-build-and-tests.sh the "linux-gcc" job is also our "let's
>> turn on every GIT_TEST_* option you've heard of" job. Whereas on the
>> GETTEXT_POISON job we just ran "make test", well, with
>> GIT_TEST_GETTEXT_POISON=true before this series, but that was the only
>> non-default, now we've got no non-defaults.
>> 
>> I can see why we'd want this on top:
>>     
>>     diff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh
>>     index 50e0b90073f..3071099eaca 100755
>>     --- a/ci/run-build-and-tests.sh
>>     +++ b/ci/run-build-and-tests.sh
>>     @@ -32,11 +32,6 @@ linux-clang)
>>             export GIT_TEST_DEFAULT_HASH=sha256
>>             make test
>>             ;;
>>     -linux-gcc-4.8)
>>     -       # Don't run the tests; we only care about whether Git can be
>>     -       # built with GCC 4.8, as it errors out on some undesired (C99)
>>     -       # constructs that newer compilers seem to quietly accept.
>>     -       ;;
>>      *)
>>             make test
>>             ;;
>> 
>> Because there we're skipping tests on linux-gcc-4.8, but on travis it's
>> the only Ubuntu Trusty image (default is Xenial), so yes we build with
>> gcc 4.8, but we're also missing out in seeing if our tests are working
>> on a slightly older Ubuntu.
>
> The above comment you propose to remove explains why you should not
> remove it...  And, again, running the test suite doesn't seem to worth
> the extra runtime.

How is it extra runtime in any meaningful sense?

Yes, we spend more CPU time, but as far as I can tell (and I'm looking
at an ongoing run on GitHub now) these jobs are run concurrently. Some
of that was covered in the quoted paragraph below that you didn't reply
to.

I.e. if I push a commit I don't care if 10 minutes of CPU time are being
wasted by a machine sitting in some rack somewhere, the important part
is that I may be saving 20 minutes of my time, since the faster 10
minute test (running concurrently with the ~30m tests) might fail
earlier.

Or are there some job limits involved here that I'm missing?

As I was writing this the CI job for GETTEXT_POISON just finished in
10m16s showing OK=green, and the rest are still churning.

>> Also on GitHub's page the GETTEXT_POISON job finishes in ~10m but all
>> the other ones in ~30m, I see some run "make test" twice, as an aside is
>> there some limit on jobs but not overall CPU? Otherwise those would
>> finish faster if that was split up.
>> 
>> But "make test" twice doesn't account for a ~3x increase, so it's got to
>> be the extra tests we're running, i.e. svn tests, httpd etc.
>> 
>> It seems to me to provide value in itself to run some shorter version of
>> the CI for people using it as a poor man's edit/push/ci/edit loop.
SZEDER Gábor Feb. 3, 2021, 9:24 p.m. UTC | #7
On Wed, Feb 03, 2021 at 01:13:32PM +0100, Ævar Arnfjörð Bjarmason wrote:
> 
> On Mon, Feb 01 2021, SZEDER Gábor wrote:
> 
> > On Wed, Jan 27, 2021 at 01:47:27AM +0100, Ævar Arnfjörð Bjarmason wrote:
> >> 
> >> On Wed, Jan 20 2021, SZEDER Gábor wrote:
> >> 
> >> > On Wed, Jan 20, 2021 at 06:59:14PM +0100, Ævar Arnfjörð Bjarmason wrote:
> >> >> 
> >> >> On Tue, Jan 12 2021, SZEDER Gábor wrote:
> >> >> 
> >> >> > On Mon, Jan 11, 2021 at 03:47:35PM +0100, Ævar Arnfjörð Bjarmason wrote:
> >> >> >> A subsequent commit will remove GETTEXT_POISON entirely, let's start
> >> >> >> by removing the CI jobs that enable the option.
> >> >> >> 
> >> >> >> We cannot just remove the job because the CI is implicitly depending
> >> >> >> on the "poison" job being a sort of "default" job.
> >> >> >
> >> >> > I don't understand what you mean here with a "default job" that the CI
> >> >> > is implicitly depending on.  There is certainly no such default job on
> >> >> > Travis CI, and I don't think there is one on the GitHub thing.
> >> >> 
> >> >> I'll reword this. I meant that the poison job was using the default
> >> >> compiler etc., whereas the other ones were setting custom values.
> >> >> 
> >> >> I vaguely remember some list traffic about this in the past, i.e. the
> >> >> reliance on the poison job not just for that, but also as "the default
> >> >> OS image" setup.
> >> >
> >> > Oh, I didn't mean that the commit message should be clarified.  I
> >> > meant that the GETTEXT_POISON job can simply be deleted.  Sorry for
> >> > not being clear enough.
> >> 
> >> I still don't get it, and my patch still seems fine as it is to me. But
> >> I'm probably missing something.
> >> 
> >> I'm looking at these, both pointing at e6362826a04 (The fourth batch,
> >> 2021-01-25):
> >> 
> >>     # Currently says "Please be aware travis-ci.org will be shutting
> >>     # down in several weeks"
> >
> > ... and continues with:
> >
> >   "with all accounts migrating to travis-ci.com.  Please stay tuned
> >    here for more information."
> >
> > FWIW, I recently migrated manually, and apart from the different TLD
> > haven't noticed anything different (though somehow the jobs in my .org
> > builds sometimes took a while to start lately, but on .com they all
> > started real fast so far, as they should).
> >
> >>     https://www.travis-ci.org/github/git/git/builds/756176098
> >> 
> >>     https://github.com/git/git/actions/runs/510676142
> >> 
> >> For the GitHub one, there's only two things that are linux && gcc. The
> >> linux-gcc job, and GETTEXT_POISON (renamed to linux-gcc-default in this
> >> series).
> >> 
> >> In ci/install-dependencies.sh we install GCC 8 for linux-gcc, but also
> >> perforce, apache, git-lfs etc.
> >> 
> >> Then in ci/lib.sh we set CC=gcc-8 on linux-gcc, but retain the default
> >> on GETTEXT_POISON. We also set GIT_TEST_HTTPD=true.
> >> 
> >> So hence "linux-gcc-default", we're running as close to a default set of
> >> packages we can get and build/run git at all. I.e. just "apt install
> >> $UBUNTU_COMMON_PKGS".
> >
> > We started installing newer compilers, because we wanted to benefit
> > from their better error reporting and improved warnings.  The reason
> > we did that in existing CI jobs without adding a "default" job was
> > that we don't care enough about the default compiler and configuration
> > to justify its additional runtime, because we run the test suite
> > enough times already.
> 
> So in summary, you'd like it removed because the combination of
> GETTEXT_POISON + old compiler is no longer worth it, but while we were
> running with GETTEXT_POISON anyway we might as well run it on the old
> compiler?

Yes, though I suspect that "no one gave a damn about the compiler used
in the GETTEXT_POISON job" might be closer to the truth.  Or even "no
one gave a damn about the GETTEXT_POISON job"...

And apart from the occasional translation-related failure (i.e. 'grep
"<translated message>" actual') and one flaky test that happened to
fail in the GETTEXT_POISON job I can't recall a single case where
building and testing with the default compiler discovered an issue
that the other jobs didn't.

> > Considering that the number of CI jobs has grown since then, I would
> > rather see the build complete faster than a default job running the
> > test suite for the N+1th time.
> 
> [...]
> 
> >> Then in ci/run-build-and-tests.sh the "linux-gcc" job is also our "let's
> >> turn on every GIT_TEST_* option you've heard of" job. Whereas on the
> >> GETTEXT_POISON job we just ran "make test", well, with
> >> GIT_TEST_GETTEXT_POISON=true before this series, but that was the only
> >> non-default, now we've got no non-defaults.
> >> 
> >> I can see why we'd want this on top:
> >>     
> >>     diff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh
> >>     index 50e0b90073f..3071099eaca 100755
> >>     --- a/ci/run-build-and-tests.sh
> >>     +++ b/ci/run-build-and-tests.sh
> >>     @@ -32,11 +32,6 @@ linux-clang)
> >>             export GIT_TEST_DEFAULT_HASH=sha256
> >>             make test
> >>             ;;
> >>     -linux-gcc-4.8)
> >>     -       # Don't run the tests; we only care about whether Git can be
> >>     -       # built with GCC 4.8, as it errors out on some undesired (C99)
> >>     -       # constructs that newer compilers seem to quietly accept.
> >>     -       ;;
> >>      *)
> >>             make test
> >>             ;;
> >> 
> >> Because there we're skipping tests on linux-gcc-4.8, but on travis it's
> >> the only Ubuntu Trusty image (default is Xenial), so yes we build with
> >> gcc 4.8, but we're also missing out in seeing if our tests are working
> >> on a slightly older Ubuntu.
> >
> > The above comment you propose to remove explains why you should not
> > remove it...  And, again, running the test suite doesn't seem to worth
> > the extra runtime.
> 
> How is it extra runtime in any meaningful sense?
> 
> Yes, we spend more CPU time, but as far as I can tell (and I'm looking
> at an ongoing run on GitHub now)

(Well, I'm looking at eight ongoing builds now...)

> these jobs are run concurrently. Some
> of that was covered in the quoted paragraph below that you didn't reply
> to.
> 
> I.e. if I push a commit I don't care if 10 minutes of CPU time are being
> wasted by a machine sitting in some rack somewhere, the important part
> is that I may be saving 20 minutes of my time, since the faster 10
> minute test (running concurrently with the ~30m tests) might fail
> earlier.
> 
> Or are there some job limits involved here that I'm missing?

Travis CI usually runs 3 Linux and 2 OSX jobs in parallel, and I doubt
that GitHub would be foolish enough to run all my jobs parallel if I
were to use it.

> As I was writing this the CI job for GETTEXT_POISON just finished in
> 10m16s showing OK=green, and the rest are still churning.

Indeed, but you should also consider that:

  - At 10m16s that job is notably slower than even my tiny old laptop,
    even though it runs less tests.  If you really want to save time,
    then run 'make test' locally.

  - Without the GETTEXT_POISON job the whole build would be finished
    ~10 minutes earlier.  With six builds that's an hour.


> >> Also on GitHub's page the GETTEXT_POISON job finishes in ~10m but all
> >> the other ones in ~30m, I see some run "make test" twice, as an aside is
> >> there some limit on jobs but not overall CPU? Otherwise those would
> >> finish faster if that was split up.
> >> 
> >> But "make test" twice doesn't account for a ~3x increase, so it's got to
> >> be the extra tests we're running, i.e. svn tests, httpd etc.
> >> 
> >> It seems to me to provide value in itself to run some shorter version of
> >> the CI for people using it as a poor man's edit/push/ci/edit loop.
>
diff mbox series

Patch

diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml
index aef6643648..8b52df200f 100644
--- a/.github/workflows/main.yml
+++ b/.github/workflows/main.yml
@@ -286,7 +286,7 @@  jobs:
           - jobname: osx-gcc
             cc: gcc
             pool: macos-latest
-          - jobname: GETTEXT_POISON
+          - jobname: linux-gcc-default
             cc: gcc
             pool: ubuntu-latest
     env:
diff --git a/.travis.yml b/.travis.yml
index 05f3e3f8d7..908330a0a3 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -16,7 +16,7 @@  compiler:
 
 matrix:
   include:
-    - env: jobname=GETTEXT_POISON
+    - env: jobname=linux-gcc-default
       os: linux
       compiler:
       addons:
diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh
index 0229a77f7d..79c0633a18 100755
--- a/ci/install-dependencies.sh
+++ b/ci/install-dependencies.sh
@@ -72,7 +72,7 @@  Documentation)
 	test -n "$ALREADY_HAVE_ASCIIDOCTOR" ||
 	sudo gem install --version 1.5.8 asciidoctor
 	;;
-linux-gcc-4.8|GETTEXT_POISON)
+linux-gcc-default|linux-gcc-4.8)
 	sudo apt-get -q update
 	sudo apt-get -q -y install $UBUNTU_COMMON_PKGS
 	;;
diff --git a/ci/lib.sh b/ci/lib.sh
index 38c0eac351..d848c036c5 100755
--- a/ci/lib.sh
+++ b/ci/lib.sh
@@ -220,8 +220,7 @@  osx-clang|osx-gcc)
 	# Travis CI OS X
 	export GIT_SKIP_TESTS="t9810 t9816"
 	;;
-GETTEXT_POISON)
-	export GIT_TEST_GETTEXT_POISON=true
+linux-gcc-default)
 	;;
 Linux32)
 	CC=gcc