{"thread":{"id":"58979","subject":"[PATCH] ci: only run win+VS build & tests in Git for Windows' fork","startedAt":"2022-12-19T14:57:56Z","lastAt":"2022-12-20T09:35:38Z","messageCount":5,"participants":["Johannes Schindelin via GitGitGadget","Phillip Wood","Ævar Arnfjörð Bjarmason","Junio C Hamano","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"469289","messageId":"pull.1445.git.1671461414191.gitgitgadget@gmail.com","threadId":"58979","inReplyTo":null,"subject":"[PATCH] ci: only run win+VS build & tests in Git for Windows' fork","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-12-19T14:50:14Z","receivedAt":"2022-12-19T14:57:56Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin <johannes.schindelin@gmx.de>\n\nIt has been a frequent matter of contention that the win+VS jobs not\nonly take a long time to run, but are also more easily broken than the\nother jobs (because they do not use the same `Makefile`-based builds as\nall other jobs), and to make matters worse, these breakages are also\nmuch harder to diagnose and fix than other jobs', especially for\ncontributors who are happy to stay away from Windows.\n\nThe purpose of these win+VS jobs is to maintain the CMake-based build\nof Git, with the target audience being Visual Studio users on Windows\nwho are typically quite unfamiliar with `make` and POSIX shell\nscripting, but the benefit of whose expertise we want for the Git\nproject nevertheless.\n\nThe CMake support was introduced for that specific purpose, and already\nearly on concerns were raised that it would put an undue burden on\ncontributors to ensure that these jobs pass in CI, when they do not have\naccess to Windows machines (nor want to have that).\n\nThis developer's initial hope was that it would be enough to fix win+VS\nfailures and provide the changes to be squashed into contributors'\npatches, and that it would be worth the benefit of attracting\nWindows-based developers' contributions.\n\nNeither of these hopes have panned out.\n\nTo lower the frustration, and incidentally benefit from using way less\nbuild minutes, let's just not run the win+VS jobs by default, which\nappears to be the consensus of the mail thread leading up to\nhttps://lore.kernel.org/git/xmqqk0311blt.fsf@gitster.g/\n\nSince the Git for Windows project still needs to at least try to attract\nmore of said Windows-based developers, let's keep the jobs, but disable\nthem everywhere except in Git for Windows' fork. This will help because\nGit for Windows' branch thicket is \"continuously rebased\" via automation\nto the `shears/maint`, `shears/main`, `shears/next` and `shears/seen`\nbranches at https://github.com/git-for-windows/git. That way, the Git\nfor Windows project will still be notified early on about potential\nbreakages, but the Git project won't be burdened with fixing them\nanymore, which seems to be the best compromise we can get on this issue.\n\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n---\n    Explicitly make CMake-based CI failures the responsibility of \"Windows\n    folks\"\n    \n    Ævar and I have brain-stormed off-list what would be the best way to\n    resolve the mounting frustration with CI failures that are caused by\n    needing to mirror Makefile changes into the CMake-based build, a burden\n    that the Git project never wanted to bear.\n    \n    While he still wants to improve the CMake support (which will benefit\n    Git for Windows), the main driver of trying to extend CMake support to\n    Linux (which does not need it because make works very well there,\n    indeed) was said frustration with CI failures.\n    \n    A much quicker method to reduce that friction to close to nil is to\n    simply disable the win+VS jobs, which is what this proposal is about.\n    (Almost, at least, we still want to keep those job definitions and run\n    them in Git for Windows' fork to ensure that CMake-based builds still\n    work.)\n    \n    A very welcome side effect is to reduce the CI build time again, which\n    became alarmingly long as of recent, causing friction on its own.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1445%2Fdscho%2Fonly-run-win%2BVS-in-the-git-for-windows-fork-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1445/dscho/only-run-win+VS-in-the-git-for-windows-fork-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1445\n\n .github/workflows/main.yml | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/.github/workflows/main.yml b/.github/workflows/main.yml\nindex e67847a682c..8af3c67f605 100644\n--- a/.github/workflows/main.yml\n+++ b/.github/workflows/main.yml\n@@ -132,7 +132,7 @@ jobs:\n   vs-build:\n     name: win+VS build\n     needs: ci-config\n-    if: needs.ci-config.outputs.enabled == 'yes'\n+    if: github.event.repository.owner.login == 'git-for-windows' && needs.ci-config.outputs.enabled == 'yes'\n     env:\n       NO_PERL: 1\n       GIT_CONFIG_PARAMETERS: \"'user.name=CI' 'user.email=ci@git'\"\n\nbase-commit: c48035d29b4e524aed3a32f0403676f0d9128863\n-- \ngitgitgadget\n"},{"id":"469293","messageId":"1e701a54-0356-9f55-ab53-9d8bfdd93e55@dunelm.org.uk","threadId":"58979","inReplyTo":"pull.1445.git.1671461414191.gitgitgadget@gmail.com","subject":"Re: [PATCH] ci: only run win+VS build & tests in Git for Windows' fork","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-12-19T15:04:28Z","receivedAt":"2022-12-19T15:04:37Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Dscho\n\nOn 19/12/2022 14:50, Johannes Schindelin via GitGitGadget wrote:\n> From: Johannes Schindelin <johannes.schindelin@gmx.de>\n> [...] \n> This developer's initial hope was that it would be enough to fix win+VS\n> failures and provide the changes to be squashed into contributors'\n> patches, and that it would be worth the benefit of attracting\n> Windows-based developers' contributions.\n> \n> Neither of these hopes have panned out.\n\nI'm sad that those hopes have not panned out, but I think this patch is \nprobably the best way forward. It is certainly the lowest effort for \nnon-windows developers and the improvement it CI minutes is very welcome.\n\nBest Wishes\n\nPhillip\n\n> To lower the frustration, and incidentally benefit from using way less\n> build minutes, let's just not run the win+VS jobs by default, which\n> appears to be the consensus of the mail thread leading up to\n> https://lore.kernel.org/git/xmqqk0311blt.fsf@gitster.g/\n> \n> Since the Git for Windows project still needs to at least try to attract\n> more of said Windows-based developers, let's keep the jobs, but disable\n> them everywhere except in Git for Windows' fork. This will help because\n> Git for Windows' branch thicket is \"continuously rebased\" via automation\n> to the `shears/maint`, `shears/main`, `shears/next` and `shears/seen`\n> branches at https://github.com/git-for-windows/git. That way, the Git\n> for Windows project will still be notified early on about potential\n> breakages, but the Git project won't be burdened with fixing them\n> anymore, which seems to be the best compromise we can get on this issue.\n> \n> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n> ---\n>      Explicitly make CMake-based CI failures the responsibility of \"Windows\n>      folks\"\n>      \n>      Ævar and I have brain-stormed off-list what would be the best way to\n>      resolve the mounting frustration with CI failures that are caused by\n>      needing to mirror Makefile changes into the CMake-based build, a burden\n>      that the Git project never wanted to bear.\n>      \n>      While he still wants to improve the CMake support (which will benefit\n>      Git for Windows), the main driver of trying to extend CMake support to\n>      Linux (which does not need it because make works very well there,\n>      indeed) was said frustration with CI failures.\n>      \n>      A much quicker method to reduce that friction to close to nil is to\n>      simply disable the win+VS jobs, which is what this proposal is about.\n>      (Almost, at least, we still want to keep those job definitions and run\n>      them in Git for Windows' fork to ensure that CMake-based builds still\n>      work.)\n>      \n>      A very welcome side effect is to reduce the CI build time again, which\n>      became alarmingly long as of recent, causing friction on its own.\n> \n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1445%2Fdscho%2Fonly-run-win%2BVS-in-the-git-for-windows-fork-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1445/dscho/only-run-win+VS-in-the-git-for-windows-fork-v1\n> Pull-Request: https://github.com/gitgitgadget/git/pull/1445\n> \n>   .github/workflows/main.yml | 2 +-\n>   1 file changed, 1 insertion(+), 1 deletion(-)\n> \n> diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml\n> index e67847a682c..8af3c67f605 100644\n> --- a/.github/workflows/main.yml\n> +++ b/.github/workflows/main.yml\n> @@ -132,7 +132,7 @@ jobs:\n>     vs-build:\n>       name: win+VS build\n>       needs: ci-config\n> -    if: needs.ci-config.outputs.enabled == 'yes'\n> +    if: github.event.repository.owner.login == 'git-for-windows' && needs.ci-config.outputs.enabled == 'yes'\n>       env:\n>         NO_PERL: 1\n>         GIT_CONFIG_PARAMETERS: \"'user.name=CI' 'user.email=ci@git'\"\n> \n> base-commit: c48035d29b4e524aed3a32f0403676f0d9128863\n"},{"id":"469303","messageId":"221219.86zgbjxnqg.gmgdl@evledraar.gmail.com","threadId":"58979","inReplyTo":"pull.1445.git.1671461414191.gitgitgadget@gmail.com","subject":"Re: [PATCH] ci: only run win+VS build & tests in Git for Windows' fork","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-12-19T17:49:12Z","receivedAt":"2022-12-19T18:17:03Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Dec 19 2022, Johannes Schindelin via GitGitGadget wrote:\n\n> From: Johannes Schindelin <johannes.schindelin@gmx.de>\n> [...]\n>     Explicitly make CMake-based CI failures the responsibility of \"Windows\n>     folks\"\n>     \n>     Ævar and I have brain-stormed off-list what would be the best way to\n>     resolve the mounting frustration with CI failures that are caused by\n>     needing to mirror Makefile changes into the CMake-based build, a burden\n>     that the Git project never wanted to bear.\n>     \n>     While he still wants to improve the CMake support (which will benefit\n>     Git for Windows), the main driver of trying to extend CMake support to\n>     Linux (which does not need it because make works very well there,\n>     indeed) was said frustration with CI failures.\n>     \n>     A much quicker method to reduce that friction to close to nil is to\n>     simply disable the win+VS jobs, which is what this proposal is about.\n>     (Almost, at least, we still want to keep those job definitions and run\n>     them in Git for Windows' fork to ensure that CMake-based builds still\n>     work.)\n>     \n>     A very welcome side effect is to reduce the CI build time again, which\n>     became alarmingly long as of recent, causing friction on its own.\n>\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1445%2Fdscho%2Fonly-run-win%2BVS-in-the-git-for-windows-fork-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1445/dscho/only-run-win+VS-in-the-git-for-windows-fork-v1\n> Pull-Request: https://github.com/gitgitgadget/git/pull/1445\n>\n>  .github/workflows/main.yml | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml\n> index e67847a682c..8af3c67f605 100644\n> --- a/.github/workflows/main.yml\n> +++ b/.github/workflows/main.yml\n> @@ -132,7 +132,7 @@ jobs:\n>    vs-build:\n>      name: win+VS build\n>      needs: ci-config\n> -    if: needs.ci-config.outputs.enabled == 'yes'\n> +    if: github.event.repository.owner.login == 'git-for-windows' && needs.ci-config.outputs.enabled == 'yes'\n>      env:\n>        NO_PERL: 1\n>        GIT_CONFIG_PARAMETERS: \"'user.name=CI' 'user.email=ci@git'\"\n\nIn terms of implementation: I would like this to use ci-config.\n\nYes, it will take us some CI time just to spin up the image to decide on\nthat (only a few seconds), but it's preferrable to hardcoding\ngithub.com/git-for-windows, as you'll be able to opt-in to this to test\nchanges.\n\nThe outstanding \"tb/ci-concurrency\" topic shows how to do that (and\ntweaks an earlier submission of yours where it was similarly hardcoded).\n\n> The purpose of these win+VS jobs is to maintain the CMake-based build\n> of Git, with the target audience being Visual Studio users on Windows\n> who are typically quite unfamiliar with `make` and POSIX shell\n\nI thought the initial purpose of it was to test compiling & testing with\nMSVC rather than GCC?\n\nIt's only after 4c2c38e800f (ci: modification of main.yml to use cmake\nfor vs-build job, 2020-06-26) that it got co-opted to test cmake too.\n\nWhatever we do about the job at this point not discussing 4c2c38e800f in\nthe commit seems like a big omission, why not revert & adjust it if the\ncmake part of it is a perceived hassle?\n\nNow, I don't think that it's worth spending time on running two sets of\nWindows tests all the time just to tease out GCC v.s. MSVC differences,\nbut I think we should still run the build on both. That takes around\n~5m, and seems worth it to avoid and flag any portability issues.\n\nBefore this we test on gcc/clang/msvc, after this we'll have dropped\nthat last one. Whatever we do about the tests and/or cmake I think we\nreally should avoid compiler monoculture, and fix portability issues\nearly.\n\n> A very welcome side effect is to reduce the CI build time again, which\n> became alarmingly long as of recent, causing friction on its own.\n\nI think this is a good goal, but what does the \"as of recent\" refer to\nhere? When the ASAN job was introduced?\n"},{"id":"469336","messageId":"xmqqv8m6zyjv.fsf@gitster.g","threadId":"58979","inReplyTo":"pull.1445.git.1671461414191.gitgitgadget@gmail.com","subject":"Re: [PATCH] ci: only run win+VS build & tests in Git for Windows' fork","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-12-20T00:52:36Z","receivedAt":"2022-12-20T00:54:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" <gitgitgadget@gmail.com>\nwrites:\n\n>     Ævar and I have brain-stormed off-list what would be the best way to\n>     resolve the mounting frustration with CI failures that are caused by\n>     needing to mirror Makefile changes into the CMake-based build, a burden\n>     that the Git project never wanted to bear.\n\nThanks, both.\n\n>     A very welcome side effect is to reduce the CI build time again, which\n>     became alarmingly long as of recent, causing friction on its own.\n\n;-)\n"},{"id":"469359","messageId":"6c26c788-6fa9-da89-700f-f589ea4eba49@gmx.de","threadId":"58979","inReplyTo":"221219.86zgbjxnqg.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH] ci: only run win+VS build & tests in Git for Windows' fork","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-12-20T09:35:04Z","receivedAt":"2022-12-20T09:35:38Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Ævar,\n\nOn Mon, 19 Dec 2022, Ævar Arnfjörð Bjarmason wrote:\n\n> On Mon, Dec 19 2022, Johannes Schindelin via GitGitGadget wrote:\n>\n> > diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml\n> > index e67847a682c..8af3c67f605 100644\n> > --- a/.github/workflows/main.yml\n> > +++ b/.github/workflows/main.yml\n> > @@ -132,7 +132,7 @@ jobs:\n> >    vs-build:\n> >      name: win+VS build\n> >      needs: ci-config\n> > -    if: needs.ci-config.outputs.enabled == 'yes'\n> > +    if: github.event.repository.owner.login == 'git-for-windows' && needs.ci-config.outputs.enabled == 'yes'\n> >      env:\n> >        NO_PERL: 1\n> >        GIT_CONFIG_PARAMETERS: \"'user.name=CI' 'user.email=ci@git'\"\n>\n> In terms of implementation: I would like this to use ci-config.\n\nI really do not see any value in that.\n\nIf you _really_ want to run those tests, you can easily add a commit on\ntop of your branch.\n\nIt's not like you always want to run the win+VS tests on a certain subset\nof your branches.\n\nBut. A big part of the reason for this patch is to codify that it\nis _not_ the responsibility of core Git contributors to take care of these\ntests. Trying to do this via the utterly convoluted and under-documented\n`ci-config` would only water down this quite clear statement.\n\n> The outstanding \"tb/ci-concurrency\" topic shows how to do that (and\n> tweaks an earlier submission of yours where it was similarly hardcoded).\n\nI fear that the `tb/ci-concurrency` topic is a good example of \"death by\ncommittee\". Let's put in a collective effort to avoid that here.\n\n> > The purpose of these win+VS jobs is to maintain the CMake-based build\n> > of Git, with the target audience being Visual Studio users on Windows\n> > who are typically quite unfamiliar with `make` and POSIX shell\n>\n> I thought the initial purpose of it was to test compiling & testing with\n> MSVC rather than GCC?\n\nThat might have been the motivation, but there have been preciously few\npatches coming in from that side.\n\nSo few, in fact, that the question \"is it worth spending all of that\nenergy to run these builds and tests all the time\" most likely has to be\nanswered with a resounding \"No\".\n\nI can think of just two bugs that were identified in the MSVC builds, one\nwhere we had to change some code to explicitly use a stable sort (which\nMSVC would otherwise not have used), and one where we now avoid an\nunsigned/signed comparison where MSVC cast the signed value to a\nnow-insanely-large unsigned, i.e. in the wrong direction.\n\nThat's two bugs identified in how many years?\n\nWe still can catch those bugs in git-for-windows/git. And avoid some of\nthe long build times.\n\n> > A very welcome side effect is to reduce the CI build time again, which\n> > became alarmingly long as of recent, causing friction on its own.\n>\n> I think this is a good goal, but what does the \"as of recent\" refer to\n> here? When the ASAN job was introduced?\n\nIt's not only `linux-asan`. The overall build time of every single job has\ngone up. I picked randomly two runs that were roughly 6 months apart,\nhttps://github.com/git/git/actions/runs/2537915353 and\nhttps://github.com/git/git/actions/runs/3728047162. As an indicator, the\n`osx-gcc` job took 35m half a year ago, now it takes 40m. The overall time\nwent up from 7h40 (which is already _quite_ a cost!) to 8h01.\n\nI don't think that we, collectively, are judicious enough about the\nbalance between cost and benefit here.\n\nHaving said that, there is a silver lining: while the linux-asan job is\nnew and takes a whopping 44m to complete, the difference between the total\nrun time 6 months ago and today is less than that, so we must have saved\non _something_, _somewhere_. Or GitHub bought faster hardware.\n\nCiao,\nJohannes\n"}]}