| Message ID | 3e9ccffc7474698947bdcb6d49b5d0728deadd08.1741256780.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 4478ad37a7d233b8db4d46dd563ece0bc8b00af4 |
| Headers | show |
| Series | Hot fixes from Git for Windows v2.49.0-rc0 | expand |
On Thu, Mar 06, 2025 at 10:26:18AM +0000, Johannes Schindelin via GitGitGadget wrote: > From: Johannes Schindelin <johannes.schindelin@gmx.de> > Now, why does this not trigger in CI? The answer is as simple as it is > puzzling: The `win+Meson` job completely side-steps Git for Windows' > development environment, opting instead to use the GCC that is on the > `PATH` in GitHub-hosted `windows-latest` runners. That GCC is pinned to > v12.2.0 and targets the UCRT (unlikely to change any time soon, see > https://github.com/actions/runner-images/blob/win25/20250303.1/images/windows/toolsets/toolset-2022.json#L132-L141). > That is in stark contrast to Git for Windows, which uses GCC v14.2.0 and > targets MSVCRT. Git for Windows' `Makefile`-based build also obviously > uses different compiler flags, otherwise this compile error would have > had plenty of opportunity in almost 14 years to surface. Oh, interesting. I didn't even know that the Windows runners had GCC in their PATH, and thus I didn't expect it to use that compiler at all. On GitLab for example we can see that it uses the MSVC compiler as I did expect [1]: Activating VS 17.10.2 C compiler for the host machine: cl (msvc 19.40.33811 "Microsoft (R) C/C++ Optimizing Compiler Version 19.40.33811 for x64") C linker for the host machine: link link 14.40.33811.0 But you're right, on GitHub that's not the case: C compiler for the host machine: gcc (gcc 12.2.0 "gcc (x86_64-posix-seh-rev2, Built by MinGW-W64 project) 12.2.0") C linker for the host machine: gcc ld.bfd 2.39 We can easily fix that by passing the `--vsenv` flag to Meson. I'll send a patch soonish. Patrick [1]: https://gitlab.com/gitlab-org/git/-/jobs/9324989037#L95 [2]: https://github.com/git/git/actions/runs/13686408338/job/38270746786#step:5:15
Hi Patrick, On Thu, 6 Mar 2025, Patrick Steinhardt wrote: > On Thu, Mar 06, 2025 at 10:26:18AM +0000, Johannes Schindelin via GitGitGadget wrote: > > From: Johannes Schindelin <johannes.schindelin@gmx.de> > > Now, why does this not trigger in CI? The answer is as simple as it is > > puzzling: The `win+Meson` job completely side-steps Git for Windows' > > development environment, opting instead to use the GCC that is on the > > `PATH` in GitHub-hosted `windows-latest` runners. That GCC is pinned to > > v12.2.0 and targets the UCRT (unlikely to change any time soon, see > > https://github.com/actions/runner-images/blob/win25/20250303.1/images/windows/toolsets/toolset-2022.json#L132-L141). > > That is in stark contrast to Git for Windows, which uses GCC v14.2.0 and > > targets MSVCRT. Git for Windows' `Makefile`-based build also obviously > > uses different compiler flags, otherwise this compile error would have > > had plenty of opportunity in almost 14 years to surface. > > Oh, interesting. I didn't even know that the Windows runners had GCC in > their PATH, and thus I didn't expect it to use that compiler at all. On > GitLab for example we can see that it uses the MSVC compiler as I did > expect [1]: > > Activating VS 17.10.2 > C compiler for the host machine: cl (msvc 19.40.33811 "Microsoft (R) C/C++ Optimizing Compiler Version 19.40.33811 for x64") > C linker for the host machine: link link 14.40.33811.0 > > But you're right, on GitHub that's not the case: > > C compiler for the host machine: gcc (gcc 12.2.0 "gcc (x86_64-posix-seh-rev2, Built by MinGW-W64 project) 12.2.0") > C linker for the host machine: gcc ld.bfd 2.39 > > We can easily fix that by passing the `--vsenv` flag to Meson. I'll send > a patch soonish. > > Patrick > > [1]: https://gitlab.com/gitlab-org/git/-/jobs/9324989037#L95 > [2]: https://github.com/git/git/actions/runs/13686408338/job/38270746786#step:5:15 Please do not invest more time on the Visual Studio support via Meson. No contributor will use this, and I want to stop spending my time on this. The user experience of configuring a Visual Studio build via Meson is just too weak compared to the ease of CMake-based builds, and while not many Visual Studio users are familiar with CMake, even dramatically less will even so much as know about Meson. I plan on dropping all pretense that Git supports Visual Studio-based contributions soon after v2.49.0 comes out, e.g. by deleting the CMake definition and also deleting whatever Meson-specific stuff I can get away deleting in Git for Windows. It was not worth the time I invested. Ciao, Johannes
"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> writes: > In other words, contrary to my expectations, the `win+Meson` job is > ill-equipped to replace the `win build` job because it exercises a > completely different tool version/compiler flags vector than what Git > for Windows needs. It is apparent that meson support is a new procedure to build our codebase that is untested and unproven on Windows at all, given that among all people who may have stake in Windows you are discovering problems in it this late in the cycle. Nobody knows what other breakages, other than something obvious and easy to catch like "ah, compiler refuses to go further", are lurking under the radar. I would be reluctant to trust the build artifact out of meson-based build on Windows after seeing your report, especially the above part. A reasonable alternative may be to declare that meson-based build is not ready yet at this point, and possibly disable win+Meson jobs to punt and divert our engineering resources elsewhere in the meantime. For a new thing, having an uneven support depending on the platform early in the evolution is not unusual or to be ashamed of. > Nevertheless, there is currently this huge push, including breaking > changes after -rc1 and all, for switching to Meson. Therefore, we need > to make it work, somehow, even in Git for Windows' SDK, hence this > patch, at this point in time. As I said earlier already, I do not mind turning the type of this pointer, which is only used to read from a struct member, like this patch does. It is the right thing to do, so I'll apply. But I personally would not be comfortable with the product built with "completely different tool version/compiler flags vector than what G4W needs", even the compilation passes with just this small change. If I were using Windows, that is. > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> > --- > ident.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Thanks, will apply. > diff --git a/ident.c b/ident.c > index caf41fb2a98..967895d8850 100644 > --- a/ident.c > +++ b/ident.c > @@ -59,7 +59,7 @@ static struct passwd *xgetpwuid_self(int *is_bogus) > > static void copy_gecos(const struct passwd *w, struct strbuf *name) > { > - char *src; > + const char *src; > > /* Traditionally GECOS field had office phone numbers etc, separated > * with commas. Also & stands for capitalized form of the login name.
On Thu, Mar 06, 2025 at 08:33:43AM -0800, Junio C Hamano wrote: > "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> > writes: > > > In other words, contrary to my expectations, the `win+Meson` job is > > ill-equipped to replace the `win build` job because it exercises a > > completely different tool version/compiler flags vector than what Git > > for Windows needs. > > It is apparent that meson support is a new procedure to build our > codebase that is untested and unproven on Windows at all, given that > among all people who may have stake in Windows you are discovering > problems in it this late in the cycle. Nobody knows what other > breakages, other than something obvious and easy to catch like "ah, > compiler refuses to go further", are lurking under the radar. > > I would be reluctant to trust the build artifact out of meson-based > build on Windows after seeing your report, especially the above > part. > > A reasonable alternative may be to declare that meson-based build is > not ready yet at this point, and possibly disable win+Meson jobs to > punt and divert our engineering resources elsewhere in the meantime. > For a new thing, having an uneven support depending on the platform > early in the evolution is not unusual or to be ashamed of. I think it would be a bit sad to disable those jobs. They build and pass the test suite alright in Git itself, even though they fail downstream in Git for Windows. They help me quite a bit to ensure that I don't regress anything that already is working while I'm iterating on the current set of features. So in the end, I view them more as testing more variants of Windows than replacing what we currently have, similar to how we test Git on different Linux distributions. I have said before that I'm very willing to help to figure out any issues, regardless of which platform, and I stand by that statement -- if you see anything that is broken in this context and report the issue to me I'll jump on it immediately. > > Nevertheless, there is currently this huge push, including breaking > > changes after -rc1 and all, for switching to Meson. Therefore, we need > > to make it work, somehow, even in Git for Windows' SDK, hence this > > patch, at this point in time. > > As I said earlier already, I do not mind turning the type of this > pointer, which is only used to read from a struct member, like this > patch does. It is the right thing to do, so I'll apply. > > But I personally would not be comfortable with the product built > with "completely different tool version/compiler flags vector than > what G4W needs", even the compilation passes with just this small > change. If I were using Windows, that is. That's completely fair. The CI job we have isn't meant to verify that we have a G4W-compatible distribution falling out of it, it merely verifies that we can build and pass tests in such a "standalone" (that is, without the SDK) configuration. We might eventually want to introduce another job that _does_ use the SDK with Meson, as well, but I didn't yet see a need for that until now. Patrick
Patrick Steinhardt <ps@pks.im> writes: > I think it would be a bit sad to disable those jobs. They build and pass > the test suite alright in Git itself, even though they fail downstream > in Git for Windows. They help me quite a bit to ensure that I don't > regress anything that already is working while I'm iterating on the > current set of features. So in the end, I view them more as testing more > variants of Windows than replacing what we currently have, similar to > how we test Git on different Linux distributions. Hmph, but compared to Linux or macOS platforms, do developers on Windows (and users of Git on Windows, including but not limited to users of Git-for-windows) benefit from having the code base to be tested on "more variants of Windows", or would it be more noise that they need to go visit the failing CI and spend time to triage if the breakage is something they should worry about? The above is more or less a rhetorical question. I think by now everybody knows I do not like monoculture, and if we had infinite engineering resources, I would think it would be healthy to have more than one prominent and competing Windows port of Git (no, I know about Cygwin, but I hear that the platform is POSIXy enough, so I do not exactly consider Git running on Cygwin qualifies as "a Windows port"). But we do not seem to live in such a world. > I have said before that I'm very willing to help to figure out any > issues, regardless of which platform, and I stand by that > statement -- if you see anything that is broken in this context > and report the issue to me I'll jump on it immediately. It's ultimately up to Windows folks to take you up on the offer. >> > Nevertheless, there is currently this huge push, including breaking >> > changes after -rc1 and all, for switching to Meson. Therefore, we need >> > to make it work, somehow, even in Git for Windows' SDK, hence this >> > patch, at this point in time. >> ... > That's completely fair. The CI job we have isn't meant to verify that we > have a G4W-compatible distribution falling out of it, it merely verifies > that we can build and pass tests in such a "standalone" (that is, > without the SDK) configuration. We might eventually want to introduce > another job that _does_ use the SDK with Meson, as well, but I didn't > yet see a need for that until now. Knowing that it is the most widely used platform, I naturally and naïvely was expecting and hoping that there are folks other than Dscho who have enough bandwidth and inclination to help in this area, but so far, having a set of jobs in CI that use a toolchain that is different from what G4W uses (as expected) did not quite help X-<. Nobody noticed it until the last minute. Which made me say that we do not seem to live in such a world, which in turn makes me accept that putting all Windows eggs in a single basket and watch it closely may be a reasonable alternative when it comes to Windows [*], than hoping that diverse set of different builds eventually help flourishing Windows developer community. I dunno. [Footnote] * Yes, I admit that it may be another way to say that I do not care the particular platform deeply enough.
diff --git a/ident.c b/ident.c index caf41fb2a98..967895d8850 100644 --- a/ident.c +++ b/ident.c @@ -59,7 +59,7 @@ static struct passwd *xgetpwuid_self(int *is_bogus) static void copy_gecos(const struct passwd *w, struct strbuf *name) { - char *src; + const char *src; /* Traditionally GECOS field had office phone numbers etc, separated * with commas. Also & stands for capitalized form of the login name.