| Message ID | patch-v3-25.29-1268792233f-20220413T194847Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | CI: run "make" in CI "steps", improve UX | expand |
On 4/13/22 3:51 PM, Ævar Arnfjörð Bjarmason wrote: > Rather than pass a "$CC" and "$CC_PACKAGE" in the environment to be > picked up in ci/lib.sh let's instead have ci/lib.sh itself add it > directly to MAKEFLAGS. > > Setting CC=gcc by default made for confusing trace output, and since a > preceding change to carry it and others over across "steps" in the > GitHub CI it's been even more misleading. E.g. the "win+VS build" job > confusingly has CC=gcc set, even though it builds with MSVC. > > Let's instead reply on the Makefile default of CC=cc, and only > override it for those jobs where it's needed. This does mean that > we'll need to set it for the "pedantic" job, which previously relied > on the default CC=gcc in case "clang" become the default on that > platform. s/reply/rely/ > This partially reverts my 707d2f2fe86 (CI: use "$runs_on_pool", not > "$jobname" to select packages & config, 2021-11-23), i.e. we're now > aiming to only set those variables specific jobs need. > > Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
On Wed, Apr 13, 2022 at 12:52 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: > @@ -160,13 +169,23 @@ linux-TEST-vars) > setenv --test GIT_TEST_WRITE_REV_INDEX 1 > setenv --test GIT_TEST_CHECKOUT_WORKERS 2 > ;; > +osx-gcc) > + CC=gcc > + CC_PACKAGE=gcc-9 not sure when this was broken since there were too many refactorings around this code, but this is definitely wrong. macos' gcc is really clang, so if we really want to build with gcc instead (and there are 9, 10 and 11 versions installed) need to use instead (for version 9, which was what was used originally and what CC_PACKAGE was installing if needed) CC=gcc-9 Right now both macos jobs are using clang, regardless of what the nicely named label says. Carlo
On Thu, Apr 14 2022, Carlo Arenas wrote: > On Wed, Apr 13, 2022 at 12:52 PM Ævar Arnfjörð Bjarmason > <avarab@gmail.com> wrote: >> @@ -160,13 +169,23 @@ linux-TEST-vars) >> setenv --test GIT_TEST_WRITE_REV_INDEX 1 >> setenv --test GIT_TEST_CHECKOUT_WORKERS 2 >> ;; >> +osx-gcc) >> + CC=gcc >> + CC_PACKAGE=gcc-9 > > not sure when this was broken since there were too many refactorings > around this code, but this is definitely wrong. > > macos' gcc is really clang, so if we really want to build with gcc > instead (and there are 9, 10 and 11 versions installed) need to use > instead (for version 9, which was what was used originally and what > CC_PACKAGE was installing if needed) > > CC=gcc-9 > > Right now both macos jobs are using clang, regardless of what the > nicely named label says. I didn't know gcc on OSX was clang, that does seem broken. But unless I'm missing something that's already been the case on "master" for a while, i.e. this is the master run showing that we'll invoke "gcc": https://github.com/git/git/runs/6031562726?check_suite_focus=true#step:3:6 And "seen", with this change, which shows that we'll do the same: https://github.com/git/git/runs/6031564900?check_suite_focus=true#step:5:7 If I understand you correctly both are effectively a NOOP and we don't use that "gcc-9", but we should. It looks like it was broken in my 707d2f2fe86 (CI: use "$runs_on_pool", not "$jobname" to select packages & config, 2021-11-23). I'll fix that as along with any small follow-ups after this series, sound good?
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > I'll fix that as along with any small follow-ups after this series, > sound good? Without any small follow-ups would be much much more preferrrable, and leave the follow-up to a separate series. A series doing too many things at once can mix bad apples slip in by overwhelming the reviewer bandwidth, and I am afraid that it may be what happened in the original "bunch of Makefile updates" that the bug originated from. I am guessing that Carlo meant the same when he said "since there were too many refactorings around this code". Thanks.
On Fri, Apr 15, 2022 at 6:52 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: > > But unless I'm missing something that's already been the case on > "master" for a while, i.e. this is the master run showing that we'll > invoke "gcc": > https://github.com/git/git/runs/6031562726?check_suite_focus=true#step:3:6 That would be unfortunate, considering the code in next (and all other not yet affected) branches still has the definition added in 176441bfb58 (ci: build Git with GCC 9 in the 'osx-gcc' build job, 2019-11-27) and which survived your travis-ci removal series. Note that the value of CC is only relevant when the code is built, and GitHub actions provides its own for all tasks and at least this "mock" run (which adds `$CC --version` before make) with next seems to indicate the right compiler is still being used there and hopefully also in all other unaffected runs from all stable branches: https://github.com/carenas/git/runs/6041742809 Eitherway, making sure that we really use gcc in the jobs that are tagged as such will make these extra runs (and their additional CPU time) less wasteful and should be corrected ASAP. Carlo
On Fri, Apr 15, 2022 at 12:03 PM Carlo Arenas <carenas@gmail.com> wrote: > > On Fri, Apr 15, 2022 at 6:52 AM Ævar Arnfjörð Bjarmason > <avarab@gmail.com> wrote: > > > > But unless I'm missing something that's already been the case on > > "master" for a while, i.e. this is the master run showing that we'll > > invoke "gcc": > > https://github.com/git/git/runs/6031562726?check_suite_focus=true#step:3:6 > > That would be unfortunate, considering the code in next (and all other > not yet affected) branches still have the definition added in > 176441bfb58 (ci: build Git with GCC 9 in the 'osx-gcc' build job, > 2019-11-27) and which survived your travis-ci removal series. Nevermind, my test was flawed, this[1] updated run does show this code was indeed refactored away and the bug also introduced to next and beyond. Carlo [1] https://github.com/carenas/git/runs/6042506530
diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index 0787cadc76b..6d25ec4ae3b 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -238,37 +238,24 @@ jobs: matrix: vector: - jobname: linux-clang - cc: clang pool: ubuntu-latest - jobname: linux-sha256 - cc: clang os: ubuntu pool: ubuntu-latest - jobname: linux-gcc - cc: gcc - cc_package: gcc-8 pool: ubuntu-latest - jobname: linux-TEST-vars - cc: gcc os: ubuntu - cc_package: gcc-8 pool: ubuntu-latest - jobname: osx-clang - cc: clang pool: macos-latest - jobname: osx-gcc - cc: gcc - cc_package: gcc-9 pool: macos-latest - jobname: linux-gcc-default - cc: gcc pool: ubuntu-latest - jobname: linux-leaks - cc: gcc pool: ubuntu-latest env: - CC: ${{matrix.vector.cc}} - CC_PACKAGE: ${{matrix.vector.cc_package}} jobname: ${{matrix.vector.jobname}} runs_on_pool: ${{matrix.vector.pool}} runs-on: ${{matrix.vector.pool}} diff --git a/ci/lib.sh b/ci/lib.sh index 8fb0bfd43e1..c73b107d9c7 100755 --- a/ci/lib.sh +++ b/ci/lib.sh @@ -51,6 +51,10 @@ setenv () { fi } +# Clear variables that may come from the outside world. +CC= +CC_PACKAGE= + # How many jobs to run in parallel? NPROC=10 @@ -70,8 +74,6 @@ MAKEFLAGS="$MAKEFLAGS SKIP_DASHED_BUILT_INS=$SKIP_DASHED_BUILT_INS" case "$CI_TYPE" in github-actions) - CC="${CC:-gcc}" - setenv --test GIT_PROVE_OPTS "--timer --jobs $NPROC" GIT_TEST_OPTS="--verbose-log -x" test Windows != "$RUNNER_OS" || @@ -143,9 +145,16 @@ vs-test) setenv --test MAKEFLAGS "$COMMON_MAKEFLAGS" ;; linux-gcc) + CC=gcc + CC_PACKAGE=gcc-8 setenv --test GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME main ;; +linux-gcc-default) + CC=gcc + ;; linux-TEST-vars) + CC=gcc + CC_PACKAGE=gcc-8 setenv --test GIT_TEST_SPLIT_INDEX yes setenv --test GIT_TEST_MERGE_ALGORITHM recursive setenv --test GIT_TEST_FULL_IN_PACK_ARRAY true @@ -160,13 +169,23 @@ linux-TEST-vars) setenv --test GIT_TEST_WRITE_REV_INDEX 1 setenv --test GIT_TEST_CHECKOUT_WORKERS 2 ;; +osx-gcc) + CC=gcc + CC_PACKAGE=gcc-9 + ;; +osx-clang) + CC=clang + ;; linux-clang) + CC=clang setenv --test GIT_TEST_DEFAULT_HASH sha1 ;; linux-sha256) + CC=clang setenv --test GIT_TEST_DEFAULT_HASH sha256 ;; pedantic) + CC=gcc # Don't run the tests; we only care about whether Git can be # built. setenv --build DEVOPTS pedantic @@ -181,9 +200,11 @@ linux-musl) MAKEFLAGS="$MAKEFLAGS GIT_TEST_UTF8_LOCALE=C.UTF-8" ;; linux-leaks) + CC=gcc setenv --build SANITIZE leak setenv --test GIT_TEST_PASSING_SANITIZE_LEAK true ;; esac -setenv --build MAKEFLAGS "$MAKEFLAGS CC=${CC:-cc}" +MAKEFLAGS="$MAKEFLAGS${CC:+ CC=$CC}" +setenv --build MAKEFLAGS "$MAKEFLAGS"
Rather than pass a "$CC" and "$CC_PACKAGE" in the environment to be picked up in ci/lib.sh let's instead have ci/lib.sh itself add it directly to MAKEFLAGS. Setting CC=gcc by default made for confusing trace output, and since a preceding change to carry it and others over across "steps" in the GitHub CI it's been even more misleading. E.g. the "win+VS build" job confusingly has CC=gcc set, even though it builds with MSVC. Let's instead reply on the Makefile default of CC=cc, and only override it for those jobs where it's needed. This does mean that we'll need to set it for the "pedantic" job, which previously relied on the default CC=gcc in case "clang" become the default on that platform. This partially reverts my 707d2f2fe86 (CI: use "$runs_on_pool", not "$jobname" to select packages & config, 2021-11-23), i.e. we're now aiming to only set those variables specific jobs need. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- .github/workflows/main.yml | 13 ------------- ci/lib.sh | 27 ++++++++++++++++++++++++--- 2 files changed, 24 insertions(+), 16 deletions(-)