| Message ID | 20210816045750.36499-1-carenas@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ci: add job for gcc-4.8 to GitHub Actions | expand |
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
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
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
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.
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
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.
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
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(-)