threads / patch / 56290

patchci: add job for gcc-4.8 to GitHub Actions

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

## tl;dr

7 messages between Aug 16, 2021 and Aug 17, 2021. Diffs are folded; open one to read it.

replies: 6people: 5as markdown or json

Carlo Marcelo Arenas Belón· Aug 16, 2021, 04:57 UTC · lore

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(-)
Show changes to 2 files +8 −1

.github/workflows/main.yml, ci/install-dependencies.sh

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
Derrick Stolee· Aug 16, 2021, 16:06 UTC · re: Carlo Marcelo Arenas Belón · lore

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

On 8/16/2021 12:57 AM, Carlo Marcelo Arenas Belón wrote:
Show 18 quoted lines
> 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?

Show 11 quoted lines
> 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.
Show 20 quoted lines
>      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

Jeff King· Aug 16, 2021, 16:18 UTC · re: Derrick Stolee · lore

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

On Mon, Aug 16, 2021 at 12:06:46PM -0400, Derrick Stolee wrote:
Show 6 quoted lines
> 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
SZEDER Gábor· Aug 17, 2021, 11:15 UTC · re: Jeff King · lore

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

On Mon, Aug 16, 2021 at 12:18:52PM -0400, Jeff King wrote:
Show 16 quoted lines
> 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.

Jeff King· Aug 17, 2021, 15:15 UTC · re: SZEDER Gábor · lore

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

On Tue, Aug 17, 2021 at 01:15:12PM +0200, SZEDER Gábor wrote:
Show 14 quoted lines
> > 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
Junio C Hamano· Aug 17, 2021, 19:36 UTC · re: SZEDER Gábor · lore

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

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

Show 6 quoted lines
> 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.

Carlo Arenas· Aug 16, 2021, 16:58 UTC · re: Derrick Stolee · lore

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

On Mon, Aug 16, 2021 at 9:06 AM Derrick Stolee <stolee@gmail.com> wrote:
Show 9 quoted lines
> 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

← back to recent threads