Re: [PATCH] ci: only run win+VS build & tests in Git for Windows' fork

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Dec 19, 2022, 17:49 UTC
Message-ID: <221219.86zgbjxnqg.gmgdl@evledraar.gmail.com>
In-Reply-To: <pull.1445.git.1671461414191.gitgitgadget@gmail.com>

On Mon, Dec 19 2022, Johannes Schindelin via GitGitGadget wrote:

Show 44 quoted lines

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> [...]
>     Explicitly make CMake-based CI failures the responsibility of "Windows
>     folks"
>     
>     Ævar and I have brain-stormed off-list what would be the best way to
>     resolve the mounting frustration with CI failures that are caused by
>     needing to mirror Makefile changes into the CMake-based build, a burden
>     that the Git project never wanted to bear.
>     
>     While he still wants to improve the CMake support (which will benefit
>     Git for Windows), the main driver of trying to extend CMake support to
>     Linux (which does not need it because make works very well there,
>     indeed) was said frustration with CI failures.
>     
>     A much quicker method to reduce that friction to close to nil is to
>     simply disable the win+VS jobs, which is what this proposal is about.
>     (Almost, at least, we still want to keep those job definitions and run
>     them in Git for Windows' fork to ensure that CMake-based builds still
>     work.)
>     
>     A very welcome side effect is to reduce the CI build time again, which
>     became alarmingly long as of recent, causing friction on its own.
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1445%2Fdscho%2Fonly-run-win%2BVS-in-the-git-for-windows-fork-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1445/dscho/only-run-win+VS-in-the-git-for-windows-fork-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/1445
>
>  .github/workflows/main.yml | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml
> index e67847a682c..8af3c67f605 100644
> --- a/.github/workflows/main.yml
> +++ b/.github/workflows/main.yml
> @@ -132,7 +132,7 @@ jobs:
>    vs-build:
>      name: win+VS build
>      needs: ci-config
> -    if: needs.ci-config.outputs.enabled == 'yes'
> +    if: github.event.repository.owner.login == 'git-for-windows' && needs.ci-config.outputs.enabled == 'yes'
>      env:
>        NO_PERL: 1
>        GIT_CONFIG_PARAMETERS: "'user.name=CI' 'user.email=ci@git'"

In terms of implementation: I would like this to use ci-config.

Yes, it will take us some CI time just to spin up the image to decide on that (only a few seconds), but it's preferrable to hardcoding github.com/git-for-windows, as you'll be able to opt-in to this to test changes.

The outstanding "tb/ci-concurrency" topic shows how to do that (and tweaks an earlier submission of yours where it was similarly hardcoded).

> The purpose of these win+VS jobs is to maintain the CMake-based build
> of Git, with the target audience being Visual Studio users on Windows
> who are typically quite unfamiliar with `make` and POSIX shell

I thought the initial purpose of it was to test compiling & testing with MSVC rather than GCC?

It's only after 4c2c38e800f (ci: modification of main.yml to use cmake for vs-build job, 2020-06-26) that it got co-opted to test cmake too.

Whatever we do about the job at this point not discussing 4c2c38e800f in the commit seems like a big omission, why not revert & adjust it if the cmake part of it is a perceived hassle?

Now, I don't think that it's worth spending time on running two sets of Windows tests all the time just to tease out GCC v.s. MSVC differences, but I think we should still run the build on both. That takes around ~5m, and seems worth it to avoid and flag any portability issues.

Before this we test on gcc/clang/msvc, after this we'll have dropped that last one. Whatever we do about the tests and/or cmake I think we really should avoid compiler monoculture, and fix portability issues early.

> A very welcome side effect is to reduce the CI build time again, which
> became alarmingly long as of recent, causing friction on its own.

I think this is a good goal, but what does the "as of recent" refer to here? When the ASAN job was introduced?