[PATCH] ci: add job for gcc-4.8 to GitHub Actions

[PATCH] ci: add job for gcc-4.8 to GitHub Actions

[Carlo Marcelo Arenas Belón]

unlike the other jobs; using an older ubuntu base image that provides
that compiler as an option.

note the obsoleted travis job used an image of the OS that is EOL and
therefore not available, but the compiler used will be the same, and
more importantly will fail in the same (C89 compatibility) issues.

Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
based on top of my tip for cb/reftable-fixes, but applies cleanly all
the way to maint.

a succesful run can be seen in:

  https://github.com/carenas/git/runs/3336674183

it adds 2m to the current setup, but gcc 4.8 is hard to find in modern
developer workstations (or even non EOL enterprise systems)

 .github/workflows/main.yml | 3 +++
 ci/install-dependencies.sh | 6 +++++-
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml
index 73856bafc9..0f211173fc 100644
--- a/.github/workflows/main.yml
+++ b/.github/workflows/main.yml
@@ -297,6 +297,9 @@ jobs:
           - jobname: linux-gcc-default
             cc: gcc
             pool: ubuntu-latest
+          - jobname: linux-gcc-4.8
+            cc: gcc-4.8
+            pool: ubuntu-18.04
     env:
       CC: ${{matrix.vector.cc}}
       jobname: ${{matrix.vector.jobname}}
diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh
index 67852d0d37..950bc39129 100755
--- a/ci/install-dependencies.sh
+++ b/ci/install-dependencies.sh
@@ -72,10 +72,14 @@ Documentation)
 	test -n "$ALREADY_HAVE_ASCIIDOCTOR" ||
 	sudo gem install --version 1.5.8 asciidoctor
 	;;
-linux-gcc-default|linux-gcc-4.8)
+linux-gcc-default)
 	sudo apt-get -q update
 	sudo apt-get -q -y install $UBUNTU_COMMON_PKGS
 	;;
+linux-gcc-4.8)
+	sudo apt-get -q update
+	sudo apt-get -q -y install $UBUNTU_COMMON_PKGS gcc-4.8
+	;;
 esac
 
 if type p4d >/dev/null && type p4 >/dev/null
-- 
2.33.0.rc2.476.g1b09a32a73

Re: [PATCH] ci: add job for gcc-4.8 to GitHub Actions

[Derrick Stolee]

On 8/16/2021 12:57 AM, Carlo Marcelo Arenas Belón wrote:
> unlike the other jobs; using an older ubuntu base image that provides
> that compiler as an option.
> 
> note the obsoleted travis job used an image of the OS that is EOL and
> therefore not available, but the compiler used will be the same, and
> more importantly will fail in the same (C89 compatibility) issues.
> 
> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
> ---
> based on top of my tip for cb/reftable-fixes, but applies cleanly all
> the way to maint.
> 
> a succesful run can be seen in:
> 
>   https://github.com/carenas/git/runs/3336674183
> 
> it adds 2m to the current setup, but gcc 4.8 is hard to find in modern
> developer workstations (or even non EOL enterprise systems)

Forgive me, I probably missed a discussion about this
somewhere else on the list, but...

Could you describe why we want GCC 4.8 in our CI? Is that a
compiler version that we officially support? What kind of
syntax triggers a problem on 4.8 versus latest?

> diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml
> index 73856bafc9..0f211173fc 100644
> --- a/.github/workflows/main.yml
> +++ b/.github/workflows/main.yml
> @@ -297,6 +297,9 @@ jobs:
>            - jobname: linux-gcc-default
>              cc: gcc
>              pool: ubuntu-latest
> +          - jobname: linux-gcc-4.8
> +            cc: gcc-4.8
> +            pool: ubuntu-18.04

Makes sense.

>      env:
>        CC: ${{matrix.vector.cc}}
>        jobname: ${{matrix.vector.jobname}}
> diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh
> index 67852d0d37..950bc39129 100755
> --- a/ci/install-dependencies.sh
> +++ b/ci/install-dependencies.sh
> @@ -72,10 +72,14 @@ Documentation)
>  	test -n "$ALREADY_HAVE_ASCIIDOCTOR" ||
>  	sudo gem install --version 1.5.8 asciidoctor
>  	;;
> -linux-gcc-default|linux-gcc-4.8)
> +linux-gcc-default)
>  	sudo apt-get -q update
>  	sudo apt-get -q -y install $UBUNTU_COMMON_PKGS
>  	;;
> +linux-gcc-4.8)
> +	sudo apt-get -q update
> +	sudo apt-get -q -y install $UBUNTU_COMMON_PKGS gcc-4.8
> +	;;

Interesting that we already had a case here. Is there interesting
history about this prior-existing case that might be illuminating
to the current need?

Thanks,
-Stolee

Re: [PATCH] ci: add job for gcc-4.8 to GitHub Actions

[Jeff King]

On Mon, Aug 16, 2021 at 12:06:46PM -0400, Derrick Stolee wrote:

> Forgive me, I probably missed a discussion about this
> somewhere else on the list, but...
> 
> Could you describe why we want GCC 4.8 in our CI? Is that a
> compiler version that we officially support? What kind of
> syntax triggers a problem on 4.8 versus latest?

Try fb9d7431cf (travis-ci: build with GCC 4.8 as well, 2019-07-18).
(found with "git log -Sgcc-4.8 ci"). The gist of it is to find variable
declarations in for-loops.

IMHO it may be more trouble than it's worth. If we can't find a compiler
that complains on this construct, then maybe it is not a construct worth
worrying too much about.

-Peff

Re: [PATCH] ci: add job for gcc-4.8 to GitHub Actions

[SZEDER Gábor]

On Mon, Aug 16, 2021 at 12:18:52PM -0400, Jeff King wrote:
> On Mon, Aug 16, 2021 at 12:06:46PM -0400, Derrick Stolee wrote:
> 
> > Forgive me, I probably missed a discussion about this
> > somewhere else on the list, but...
> > 
> > Could you describe why we want GCC 4.8 in our CI? Is that a
> > compiler version that we officially support? What kind of
> > syntax triggers a problem on 4.8 versus latest?
> 
> Try fb9d7431cf (travis-ci: build with GCC 4.8 as well, 2019-07-18).
> (found with "git log -Sgcc-4.8 ci"). The gist of it is to find variable
> declarations in for-loops.
> 
> IMHO it may be more trouble than it's worth. If we can't find a compiler
> that complains on this construct, then maybe it is not a construct worth
> worrying too much about.

I for one like for loop initial declarations, because they allow us to
limit the scope of the loop variable to the loop, and would love to
see it used in more places (well, wherever possible, actually, but
that'd be a lot of churn).  So I would prefer to just drop this job
sooner rather than later, update CodingGuidelines, and, if deemed
necessary, launch a weather balloon.

Re: [PATCH] ci: add job for gcc-4.8 to GitHub Actions

[Jeff King]

On Tue, Aug 17, 2021 at 01:15:12PM +0200, SZEDER Gábor wrote:

> > Try fb9d7431cf (travis-ci: build with GCC 4.8 as well, 2019-07-18).
> > (found with "git log -Sgcc-4.8 ci"). The gist of it is to find variable
> > declarations in for-loops.
> > 
> > IMHO it may be more trouble than it's worth. If we can't find a compiler
> > that complains on this construct, then maybe it is not a construct worth
> > worrying too much about.
> 
> I for one like for loop initial declarations, because they allow us to
> limit the scope of the loop variable to the loop, and would love to
> see it used in more places (well, wherever possible, actually, but
> that'd be a lot of churn).  So I would prefer to just drop this job
> sooner rather than later, update CodingGuidelines, and, if deemed
> necessary, launch a weather balloon.

Yeah, I think it would be nice to use that, too, if it works everywhere.
The last discussion of this was from 2017, where peple likewise seemed
in favor:

  https://lore.kernel.org/git/xmqqlgnrq9qi.fsf@gitster.mtv.corp.google.com/

There was even a weather balloon patch:

  https://lore.kernel.org/git/20170719181956.15845-1-sbeller@google.com/

but it got hung up on somebody using gcc 4.8. ;)

It looks like the default flavor bumped to gnu90 in gcc 5. That came out
in 2015, but the last 4.x release was in August 2016. Which is getting
old-ish, but still well within the realm of what people may be using an
on older distro (e.g., RHEL7, which is still supported).

For gcc, at least, this is trivially fixable with a `-std` flag (though
it may be tricky to make it work out-of-the-box through the Makefile). I
don't think we'll know if there problems with other compilers until we
do the weather balloon.

-Peff

Re: [PATCH] ci: add job for gcc-4.8 to GitHub Actions

[Junio C Hamano]

SZEDER Gábor <szeder.dev@gmail.com> writes:

>> IMHO it may be more trouble than it's worth. If we can't find a compiler
>> that complains on this construct, then maybe it is not a construct worth
>> worrying too much about.

Absolutely.  Of course, there are vendor compilers that are not GNU
or clang that may throw us surprises ;-)

> I for one like for loop initial declarations, because they allow us to
> limit the scope of the loop variable to the loop, and would love to
> see it used in more places (well, wherever possible, actually, but
> that'd be a lot of churn).  So I would prefer to just drop this job
> sooner rather than later, update CodingGuidelines, and, if deemed
> necessary, launch a weather balloon.

I'd agree in general, but it must be done in a bit different order,
i.e. weather balloon first.

Re: [PATCH] ci: add job for gcc-4.8 to GitHub Actions

[Carlo Arenas]

On Mon, Aug 16, 2021 at 9:06 AM Derrick Stolee <stolee@gmail.com> wrote:
> On 8/16/2021 12:57 AM, Carlo Marcelo Arenas Belón wrote:
> > it adds 2m to the current setup, but gcc 4.8 is hard to find in modern
> > developer workstations (or even non EOL enterprise systems)
>
> Forgive me, I probably missed a discussion about this
> somewhere else on the list, but...
>
> Could you describe why we want GCC 4.8 in our CI? Is that a
> compiler version that we officially support?

couldn't find the specific thread I seem to remember, but AFAIK it was
because it is the compiler from RHEL7

Carlo