| Message ID | patch-1.1-f27d8bd4491-20221201T162451Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | test-lib.sh: discover "git" in subdirs of "contrib/buildsystems/out" | expand |
On 01/12/2022 16:39, Ævar Arnfjörð Bjarmason wrote: > Fix a regression in [1] and discover git built by cmake in subdirs of > "contrib/buildsystems/out", in addition to the "out" directory itself. How do you propose to find the most recent build? There are different directories for different architectures and different directories for debug and release builds. Hard coding the build directory is a bad idea and should be dropped. Best Wishes Phillip > As noted in [2] the default for Visual Studio is to use > "out/build/<config>", where "<config>" is a default configuration > name. We might be able to make this deterministic in the future with a > "CMakePresets.json", but that facility is newer than our oldest > supported CMake and VS version. > > 1. 16a5421a654 (Makefile + cmake: use environment, not GIT-BUILD-DIR, > 2022-11-03) > 2. https://learn.microsoft.com/en-us/cpp/build/cmake-projects-in-visual-studio?view=msvc-170 > > Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> > --- > > On Thu, Dec 01 2022, Phillip Wood wrote: > >> On 30/11/2022 10:16, Ævar Arnfjörð Bjarmason wrote: >>> On Wed, Nov 30 2022, Phillip Wood wrote: >>> >>>> Hi Junio >>> Hi, and thanks for your review of this series. >>> >>>> On 29/11/2022 09:40, Junio C Hamano wrote: >>>>> * ab/cmake-nix-and-ci (2022-11-04) 14 commits >>>>> (merged to 'next' on 2022-11-08 at 6ef4e93b36) >>>>> + CI: add a "linux-cmake-test" to run cmake & ctest on linux >>>>> + cmake: copy over git-p4.py for t983[56] perforce test >>>>> + cmake: only look for "sh" in "C:/Program Files" on Windows >>>>> + cmake: increase test timeout on Windows only >>>>> + cmake: support GIT_TEST_OPTS, abstract away WIN32 defaults >>>>> + Makefile + cmake: use environment, not GIT-BUILD-DIR >>>>> + test-lib.sh: support a "GIT_TEST_BUILD_DIR" >>>>> + cmake: set "USE_LIBPCRE2" in "GIT-BUILD-OPTIONS" for test-lib.sh >>>>> + cmake & test-lib.sh: add a $GIT_SOURCE_DIR variable >>>>> + cmake: chmod +x the bin-wrappers/* & SCRIPT_{SH,PERL} & git-p4 >>>>> + cmake: don't copy chainlint.pl to build directory >>>>> + cmake: update instructions for portable CMakeLists.txt >>>>> + cmake: use "-S" and "-B" to specify source and build directories >>>>> + cmake: don't invoke msgfmt with --statistics >>>>> Fix assorted issues with CTest on *nix machines. >> >> Junio please drop this series when you rebuild next as it breaks >> manually running individual test scripts when building with Visual >> Studio. > > I think the issue you've spotted is easily fixed on top. See below. > >>>> If that's all this series did then I think it would be fine. However >>>> it also makes changes to test-lib.sh to hard code the build directory >>>> in an attempt to remove GIT-BUILD-DIR. I'm not convinced that is an >>>> improvement on the status quo. >>> I think the series as it stands addresses those concerns. In >>> particular >>> building outside of contrib/buildsystems/out works, just as before: >>> cmake -S contrib/buildsystems -B /tmp/git-build >>> -DCMAKE_BUILD_TYPE=Debug && >>> make -C /tmp/git-build && >>> ctest --test-dir /tmp/git-build -R t0001 >>> Per [1] and [2] which added the "ctest" support that's the use-case >>> for >>> this part of the build: running the tests with ctest, which works as >>> before with the default or custom directories. >>> Perhaps the reason this has been a sticking point for you is that in >>> summarizing this, Johannes's [3] didn't make that distinction between >>> running the tests with "ctest" and running them manually by entering the >>> "t/" directory after the build. I.e.: >> >> In other words Johannes thinks both are equally important. The windows >> build has always supported running the tests manually from /t and he >> quite reasonable wants that to continue working. > > Yes, that should work. Now, clearly I missed that VS doesn't use "out" > by default, but a subdirectory, which the below patch fixes. > > But I think we can still draw a distinction between anything under > "out" and arbitrary user-supplied directories, which can possibly be > located outside of the source tree. > >>> (cd t && ./t0001-init.sh) >>> It's only that part which acts differently in this series. I.e. if >>> you >>> were to build in /tmp/git-build this would no longer find your built >>> assets: >>> $ ./t0001-init.sh >>> error: GIT-BUILD-OPTIONS missing (has Git been built?). >>> If you just leave it at the default of "contrib/buildsystems/out" >>> it'll >>> work: >>> (cd t && ./t0001-init.sh) >>> ok 1 [...] >>> I think my [4] convincingly makes the case that nobody will >>> care. I.e. as the [5] it links to the use-case for running the test >>> after the build without ctest was ("[...]" insert is mine): >>> [To build and test with VS] open the worktree as a folder, and >>> Visual Studio will find the `CMakeLists.txt` file and >>> automatically generate the project files. >>> I.e. we want to support the user who builds with that method, and >>> runs >>> the tests manually. I think you're worrying about an edge case that >>> nobody's using in practice. >> >> You seem to be assuming that Visual Studio creates its build artifacts >> in contrib/buildsystems/out based on a gitignore rule. Given the rule >> ignores _all_ subdirectories below contrib/buildsystems/out that is a >> big assumption. Despite me repeatedly raising concerns about the hard >> coded build directory you do not seem to have checked exactly where >> Visual Studio creates its build artifacts. > > I did check, but I got it wrong from reading the docs & commit message. > > I don't have a local Windows or VS setup. Thanks for testing it. > >> This morning I installed >> Visual Studio to check this and discovered the build is in a >> subdirectory below contrib/buildsystems/out so this series will break >> manual test runs for anyone building git using the recommend method. I >> find it rather frustrating that you argue below that Windows specific >> knowledge and testing are not required when you're altering the >> Windows build. > > I was saying that you could round-trip test the auto-discovery of the > build directory on *nix. > > Now, clearly I missed the "in a subdir of out" case. But that's > separate from whether you can test that case cross-platform. With the > below patch this works: > > cmake -S contrib/buildsystems -B contrib/buildsystems/out/a/b/c -DCMAKE_BUILD_TYPE=Debug && > make -C contrib/buildsystems/out/a/b/c && > (cd t && ./t0071-sort.sh) > > But did this work for you before, and does it work on "master"? I > think this never worked out of the box until my series. I.e. if you do > that on v2.38.0 (before "js/cmake-updates") you'll get: > > $ ./t0071-sort.sh > error: GIT-BUILD-OPTIONS missing (has Git been built?). > > The reason is that before "js/cmake-updates" the part of the cmake > recipe that established the ling between the test-lib.sh and the > cmake-built tree was contingent on running ctest. E.g.: > > ctest --test-dir contrib/buildsystems/out/a/b/c -R t0071 > > Once you did that it would patch your t/test-lib.sh: > > diff --git a/t/test-lib.sh b/t/test-lib.sh > index a65df2fd220..70b0a633e4c 100644 > --- a/t/test-lib.sh > +++ b/t/test-lib.sh > @@ -44,1 +44,1 @@ fi > -GIT_BUILD_DIR="${TEST_DIRECTORY%/t}" > +GIT_BUILD_DIR="$TEST_DIRECTORY/../contrib/buildsystems/out/a/b/c" > > Likewise, with "js/cmake-updates" it also doesn't work after you > *build*. I. with it it would create a "GIT-BUILD-DIR" file, but only > once ctest is run. I.e.: > > (cd t && ./t0071-sort.sh); > file GIT-BUILD-DIR; > ctest --test-dir contrib/buildsystems/out/a/b/c -R t0071 > file GIT-BUILD-DIR; > (cd t && ./t0071-sort.sh) > > Would emit: > > error: GIT-BUILD-OPTIONS missing (has Git been built?). > GIT-BUILD-DIR: cannot open `GIT-BUILD-DIR' (No such file or directory) > <ctest output> > GIT-BUILD-DIR: ASCII text, with no line terminators > <test output> > > It's only with my series that it started working wihout having to run > "ctest". With the below the test-lib.sh will optimistically find the > "git" in "contrib/buildsystems/out". > > Does the VS integration run the equivalent of "ctest" by default? > > t/test-lib.sh | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git a/t/test-lib.sh b/t/test-lib.sh > index ce319c9963e..9c63ee428f2 100644 > --- a/t/test-lib.sh > +++ b/t/test-lib.sh > @@ -57,9 +57,11 @@ if test -n "$GIT_TEST_BUILD_DIR" > then > GIT_BUILD_DIR="$GIT_TEST_BUILD_DIR" > elif ! test -x "$GIT_BUILD_DIR/git" && > - test -x "$GIT_BUILD_DIR/contrib/buildsystems/out/git" > + test -d "$GIT_BUILD_DIR/contrib/buildsystems/out" > then > - GIT_BUILD_DIR="$GIT_SOURCE_DIR/contrib/buildsystems/out" > + GIT_BUILD_OPTIONS="$(find "$GIT_BUILD_DIR/contrib/buildsystems/out" \ > + -type f -name 'GIT-BUILD-OPTIONS')" > + GIT_BUILD_DIR="${GIT_BUILD_OPTIONS%/GIT-BUILD-OPTIONS}" > GIT_AUTO_CONTRIB_BUILDSYSTEMS_OUT=t > > # On Windows, we must convert Windows paths lest they contain a colon > @@ -1646,7 +1648,7 @@ remove_trash_directory "$TRASH_DIRECTORY" || { > # anything using lib-subtest.sh > if test -n "$GIT_AUTO_CONTRIB_BUILDSYSTEMS_OUT" && test -t 1 > then > - say "setup: had no ../git, but found & used cmake built git in ../contrib/buildsystems/out/git" > + say "setup: had no ../git, but found & used cmake built git in '$GIT_BUILD_DIR/git'" > fi > > remove_trash=t
On Thu, Dec 01 2022, Phillip Wood wrote: > On 01/12/2022 16:39, Ævar Arnfjörð Bjarmason wrote: >> Fix a regression in [1] and discover git built by cmake in subdirs of >> "contrib/buildsystems/out", in addition to the "out" directory itself. > > How do you propose to find the most recent build? There are different > directories for different architectures and different directories for > debug and release builds. I'm not proposing to do that, besides, if you built locally for different architectures running the tests would just fail, wouldn't it? > Hard coding the build directory is a bad idea and should be dropped. I think it's a better idea to target the common case of a user building & being able to run the tests form t/, rather than requiring bootstrapping that process by running "ctest". Did you test that case on v2.38.0 and/or master? I.e. is VS running that ctest portion automatically?
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: >> Junio please drop this series when you rebuild next as it breaks >> manually running individual test scripts when building with Visual >> Studio. > > I think the issue you've spotted is easily fixed on top. See below. Smells more like papering over than fixed, but let's see how folks who need cmake/ctest feel about it. Let's mark the series never to graduate to 'master' for now, optionally revert it out of 'next'. Phillip, you asked about rebuilding 'next', which would not happen until 2.39.0 final---did you mean reverting the topic out of 'next'? Do you need 'next' without this topic, not just 'master'? I'll then wait for something both camps (you and folks on Visual Studio?) can agree on to requeue. Thanks.
Hi Junio On 01/12/2022 23:00, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > >>> Junio please drop this series when you rebuild next as it breaks >>> manually running individual test scripts when building with Visual >>> Studio. >> >> I think the issue you've spotted is easily fixed on top. See below. > > Smells more like papering over than fixed, but let's see how folks > who need cmake/ctest feel about it. As MSVC uses different directories for debug and release builds there can be more than one build directory. I don't think selecting one of them at random using 'find' is a good idea. > Let's mark the series never to graduate to 'master' for now, > optionally revert it out of 'next'. > > Phillip, you asked about rebuilding 'next', which would not > happen until 2.39.0 final---did you mean reverting the topic out > of 'next'? Do you need 'next' without this topic, not just > 'master'? I don't mind waiting but I'm not a Windows user. I only tested this topic under Windows because I knew Ævar had not and a quick web search for "MSVC CMake" made me worry it was broken. I'm afraid I wont be spending anymore time on this topic. I had hoped that having the CMake build work under Linux would help developers avoid breaking it. However I'm concerned that if developers do not appreciate that there are differences between the Linux and Windows builds it will actually create a false sense of security and be used as an excuse not to properly test under Windows[1]. Recent events have confirmed my view that changes like this need the attention of someone with experience of Windows development and given that yesterday was the first time I'd used MSVC since about 1994 I do not fit that description. In addition to the breakage I reported yesterday 623fde1438 (cmake: chmod +x the bin-wrappers/* & SCRIPT_{SH,PERL} & git-p4, 2022-11-03) causes CMake older that 3.19 to error out when run from MSVC because chmod does not exist on Windows. Also when running 'ctest' on "next" I see tests failing because they cannot find 'test-tool' (I haven't tried running the failing tests manually) Best Wishes Phillip [1] While our CI helps the MSVC job runs CMake manually, performs an in-tree build and does not use ctest. In contrast a user running the MSVC GUI does not run CMake themselves, ends up with an out-of-tree build and runs the tests with ctest. > I'll then wait for something both camps (you and folks on Visual > Studio?) can agree on to requeue. > > Thanks.
On Fri, Dec 02 2022, Phillip Wood wrote: > On 01/12/2022 23:00, Junio C Hamano wrote: >> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: >> >>>> Junio please drop this series when you rebuild next as it breaks >>>> manually running individual test scripts when building with Visual >>>> Studio. >>> >>> I think the issue you've spotted is easily fixed on top. See below. >> Smells more like papering over than fixed, but let's see how folks >> who need cmake/ctest feel about it. > > As MSVC uses different directories for debug and release builds there > can be more than one build directory. I don't think selecting one of > them at random using 'find' is a good idea. I think the latest iteration submitted today should fix that issue: https://lore.kernel.org/git/cover-v5-00.15-00000000000-20221202T110947Z-avarab@gmail.com/ >> Let's mark the series never to graduate to 'master' for now, >> optionally revert it out of 'next'. >> Phillip, you asked about rebuilding 'next', which would not >> happen until 2.39.0 final---did you mean reverting the topic out >> of 'next'? Do you need 'next' without this topic, not just >> 'master'? > > I don't mind waiting but I'm not a Windows user. I only tested this > topic under Windows because I knew Ævar had not and a quick web search > for "MSVC CMake" made me worry it was broken. > > I'm afraid I wont be spending anymore time on this topic. I had hoped > that having the CMake build work under Linux would help developers > avoid breaking it. However I'm concerned that if developers do not > appreciate that there are differences between the Linux and Windows > builds it will actually create a false sense of security and be used > as an excuse not to properly test under Windows[1]. Recent events have > confirmed my view that changes like this need the attention of someone > with experience of Windows development and given that yesterday was > the first time I'd used MSVC since about 1994 I do not fit that > description. > [...] > [1] While our CI helps the MSVC job runs CMake manually, performs an > in-tree build and does not use ctest. In contrast a user running the > MSVC GUI does not run CMake themselves, ends up with an out-of-tree > build and runs the tests with ctest. I don't run Windows by choice, and I'm not interested in running a propriterary IDE (VS) either. The main reason I'm working on this series is that while we as a project are happy to support proprietary OS's, it hasn't been a requirement for participation that you need to buy a copy of Windows, OSX, AIX, HP/UX or whatever to submit patches. Of course we have platform-specific code. but this CMake component is unique in how invasive it is. It's easy to e.g. stay away from the OSX-specific code in compat/fsmonitor/*darwin*.[ch], or generally speaking the Windows-specific C code. But for CMake it's become a hard requirenment for many changes, even though it's a contrib/ component. Now, I'm not looking to get rid of it, it's clearly useful, particularly with MSVC (or so I've gathered). But I'd also like some future where any time I and others who don't use Windows need to patch certain parts of the Makefile that we don't need to spend a long time bouncing thigs against the Windows CI, but can run this component other platforms.. E.g. I think for cfe853e66be (hook-list.h: add a generated list of hooks, like config-list.h, 2021-09-26) I spent at least 3-4 hours on what should have been a 5-10 minute task of making the relatively minor change to generate hook-list.h. It was a push, wait 30-60 minutes, find some minor (e.g. syntax) issue, rinse & repeat. Now, clearly the outstanding issues with (if any) need to be fixed, and thanks for sticking with testing it for so long. But hopefully the above gives you some background. > In addition to the breakage I reported yesterday 623fde1438 (cmake: > chmod +x the bin-wrappers/* & SCRIPT_{SH,PERL} & git-p4, 2022-11-03) > causes CMake older that 3.19 to error out when run from MSVC because > chmod does not exist on Windows. Also when running 'ctest' on "next" I > see tests failing because they cannot find 'test-tool' (I haven't > tried running the failing tests manually) That's odd, do you happen to have some output from "ctest" for that? Our "cmake" build already invokes shell scripts which rely on "grep", "sed" etc, and the test suite itself invokes "chmod", I just understand that it's a noop wrapper on Windows. So I can also re-roll and just ifdef that part awa on win32, but I wonder what's going on there. I wonder if it's because we'd need to spawn it via "/bin/sh" (but just skippin it seems better). One of the things I didn't change with this series is how "test-tool" is handled. I.e. it's there before in the generated "t/helper" directory, and in the generated "bin-wrappers/". We also find it when running the tests with Windows in GitHub CI. Did you try on v2.38.0 and/or "master" as well?
On Fri, Dec 02, 2022 at 05:40:34PM +0100, Ævar Arnfjörð Bjarmason wrote: > > [1] While our CI helps the MSVC job runs CMake manually, performs an > > in-tree build and does not use ctest. In contrast a user running the > > MSVC GUI does not run CMake themselves, ends up with an out-of-tree > > build and runs the tests with ctest. > > I don't run Windows by choice, and I'm not interested in running a > propriterary IDE (VS) either. > > The main reason I'm working on this series is that while we as a project > are happy to support proprietary OS's, it hasn't been a requirement for > participation that you need to buy a copy of Windows, OSX, AIX, HP/UX or > whatever to submit patches. > > Of course we have platform-specific code. but this CMake component is > unique in how invasive it is. > > It's easy to e.g. stay away from the OSX-specific code in > compat/fsmonitor/*darwin*.[ch], or generally speaking the > Windows-specific C code. > > But for CMake it's become a hard requirenment for many changes, even > though it's a contrib/ component. I have similar feelings to you here. Back when cmake support was introduced, I explicitly wanted it to be something for people who cared about it, but that wouldn't bother people who didn't use it: https://lore.kernel.org/git/20200427200852.GC1728884@coredump.intra.peff.net/ I stand by that sentiment, but it seems to have crept up as a required thing to deal with, and that is mostly because of CI. Using cmake in CI is good for telling developers when a change they make has broken cmake. But it also makes cmake their problem, and not the folks interested in cmake. Now maybe attitudes have changed, and I am out of date, and cmake support is considered mature and really important (or maybe nobody even agreed with me back then ;) ). But if not, should we consider softening the CI output so that cmake failures aren't "real" failures? That seems drastic and mean, and I don't like it. But it's the root of the issue, IMHO. As a side note, this isn't the only such instance of this problem. Two other things to think about: - You mentioned darwin fsmonitor code. And it's true that you can largely ignore it if you don't touch it. But every once in a while you get bit by it (e.g., enabling a new compiler warning which triggers in code you don't compile on your platform, and now you have to guess-and-check the fix with CI). This sucks, but is kind of inevitable on a cross-platform system. I think the issue with cmake is that because it's basically duplicating/wrapping the Makefile, it _feels_ unnecessary to people on platforms with working make, and triggers more frequently (because changes to the rest of the build system may break cmake in subtle ways). - I'd actually put the leak-checking CI in the same boat. It's a good goal, and one I hope we work towards. But it feels like the current state is not very mature, and people often end up wrestling with CI to deal with failures that they didn't even introduce (e.g., adding a new test that happens to run a Git program that has an existing leak, and now you are on the hook for figuring out why the existing "passes leaks" annotation is wrong). My original hope is that we would introduce leak-checking tooling that people interested in leaks could use, and other people could ignore until we reached a leak-free state. Because it's in CI it means that people get notified of new leaks in code they write (which is good, and helps people interested in leaks), but it also means they have to deal with the immature state. I'm not necessarily proposing to drop the leaks CI job here. I'm mostly philosophizing about the greater problem. In the early days of Git, the cross-platform testing philosophy was: somebody who cares will test on that platform and write a patch. If they don't, how important could it be? With CI that happens automatically and it becomes everybody's problem, which is a blessing and a curse. -Peff
Jeff King <peff@peff.net> writes: > I have similar feelings to you here. Back when cmake support was > introduced, I explicitly wanted it to be something for people who cared > about it, but that wouldn't bother people who didn't use it: > > https://lore.kernel.org/git/20200427200852.GC1728884@coredump.intra.peff.net/ > > I stand by that sentiment, but it seems to have crept up as a required > thing to deal with, and that is mostly because of CI. Using cmake in CI > is good for telling developers when a change they make has broken cmake. > But it also makes cmake their problem, and not the folks interested in > cmake. > > Now maybe attitudes have changed, and I am out of date, and cmake > support is considered mature and really important (or maybe nobody even > agreed with me back then ;) ). But if not, should we consider softening > the CI output so that cmake failures aren't "real" failures? That seems > drastic and mean, and I don't like it. But it's the root of the issue, > IMHO. It makes the two of us (or three couning Ævar?). > - I'd actually put the leak-checking CI in the same boat. It's a good > goal, and one I hope we work towards. But it feels like the current > state is not very mature, and people often end up wrestling with CI > to deal with failures that they didn't even introduce (e.g., adding > a new test that happens to run a Git program that has an existing > leak, and now you are on the hook for figuring out why the existing > "passes leaks" annotation is wrong). Hear, hear. > I'm not necessarily proposing to drop the leaks CI job here. I'm mostly > philosophizing about the greater problem. In the early days of Git, the > cross-platform testing philosophy was: somebody who cares will test on > that platform and write a patch. If they don't, how important could it > be? With CI that happens automatically and it becomes everybody's > problem, which is a blessing and a curse. True.
On Fri, Dec 02 2022, Jeff King wrote: > On Fri, Dec 02, 2022 at 05:40:34PM +0100, Ævar Arnfjörð Bjarmason wrote: > >> > [1] While our CI helps the MSVC job runs CMake manually, performs an >> > in-tree build and does not use ctest. In contrast a user running the >> > MSVC GUI does not run CMake themselves, ends up with an out-of-tree >> > build and runs the tests with ctest. >> >> I don't run Windows by choice, and I'm not interested in running a >> propriterary IDE (VS) either. >> >> The main reason I'm working on this series is that while we as a project >> are happy to support proprietary OS's, it hasn't been a requirement for >> participation that you need to buy a copy of Windows, OSX, AIX, HP/UX or >> whatever to submit patches. >> >> Of course we have platform-specific code. but this CMake component is >> unique in how invasive it is. >> >> It's easy to e.g. stay away from the OSX-specific code in >> compat/fsmonitor/*darwin*.[ch], or generally speaking the >> Windows-specific C code. >> >> But for CMake it's become a hard requirenment for many changes, even >> though it's a contrib/ component. > > I have similar feelings to you here. Back when cmake support was > introduced, I explicitly wanted it to be something for people who cared > about it, but that wouldn't bother people who didn't use it: > > https://lore.kernel.org/git/20200427200852.GC1728884@coredump.intra.peff.net/ > > I stand by that sentiment, but it seems to have crept up as a required > thing to deal with, and that is mostly because of CI. Using cmake in CI > is good for telling developers when a change they make has broken cmake. > But it also makes cmake their problem, and not the folks interested in > cmake. That's a bit of a pain, but I don't think the main problem is its integration with CI. It's that there doesn't really seem to be an interest in its active maintenance & review from its supposed main target audience. Case in point this "ab/cmake-nix-and-ci" topic: It's been queued for around 2 months now. I don't use Windows or VS. I'd just like it not to take half may day to throw things at Windows CI whenever some obscure aspect of cmake breaks. The only person who's been showing it any notable review interest (Phillip) apparently hasn't used Windows actively since the mid-90's :) Which is a very different situation than if it were still something that broke CI, but there was someone (or more than one person) active on list willing to test patches to get it working, was familiar with the cmake language & could help write the cmake version of Makefile changes when those were needed etc. > Now maybe attitudes have changed, and I am out of date, and cmake > support is considered mature and really important (or maybe nobody even > agreed with me back then ;) ). But if not, should we consider softening > the CI output so that cmake failures aren't "real" failures? That seems > drastic and mean, and I don't like it. But it's the root of the issue, > IMHO. Yeah, maybe. Maybe if we broke it we'd get people showing up to maintain it again :) I do think the series I've got here is the most practical way forward at this point (any outstanding issues aside). I.e. it's usually fairly easy-ish to amend the cmake recipe. It's just been taking *ages* because it's been a dumpste fire outside of Windows, so if you don't have that OS available to you you need to wait for the CI. Being able to run it in seconds on *nix really helps.. > As a side note, this isn't the only such instance of this problem. Two > other things to think about: > > - You mentioned darwin fsmonitor code. And it's true that you can > largely ignore it if you don't touch it. But every once in a while > you get bit by it (e.g., enabling a new compiler warning which > triggers in code you don't compile on your platform, and now you > have to guess-and-check the fix with CI). This sucks, but is kind of > inevitable on a cross-platform system. I think the issue with cmake > is that because it's basically duplicating/wrapping the Makefile, it > _feels_ unnecessary to people on platforms with working make, and > triggers more frequently (because changes to the rest of the build > system may break cmake in subtle ways). Yeah, we'll always have some cross-platform pain. But e.g. "chmod +x" just works in the Makefile, including when we run it on Windows. And I've run it on Windows CI. But just upthread of here Phillip is reporting that it doesn't work from the context of the CMake recipe. I've been throwing some things at Windows CI, but I'm pretty stumped as to what that might be. Some warning on Mac OS X is trivial by comparison :) > - I'd actually put the leak-checking CI in the same boat. It's a good > goal, and one I hope we work towards. But it feels like the current > state is not very mature, and people often end up wrestling with CI > to deal with failures that they didn't even introduce (e.g., adding > a new test that happens to run a Git program that has an existing > leak, and now you are on the hook for figuring out why the existing > "passes leaks" annotation is wrong). > > My original hope is that we would introduce leak-checking tooling > that people interested in leaks could use, and other people could > ignore until we reached a leak-free state. Because it's in CI it > means that people get notified of new leaks in code they write > (which is good, and helps people interested in leaks), but it also > means they have to deal with the immature state. > > I'm not necessarily proposing to drop the leaks CI job here. I'm mostly > philosophizing about the greater problem. In the early days of Git, the > cross-platform testing philosophy was: somebody who cares will test on > that platform and write a patch. If they don't, how important could it > be? With CI that happens automatically and it becomes everybody's > problem, which is a blessing and a curse. That's a definitely a bit of an irresistible digression :) First, I think we can agree that however frustrating that's been (and sorry!) it would be a lot worse if my average response time to the leak testing breaking something was measured in months :) I do think that whatever issues we've had with it (and in retrospect I'd do some of it differently) that it's a lot more mature these days than you might remember. I've been making an effort to specifically address the sorts of leaks that would cause that sort of pain, e.g. adding an unsespecting "git log" to an existing test file and the like. I have a report I generate [1] of the outstanding leaks, sorted by de-duping stack traces, and an estimate of how many tests would start passing if a combination of leaks were solved[1]. On the one hand it's ~60k lines of scary stack traces, but on the other hand it used to be easily 3-4x that (I can't recall the exact size offhand). But more importantly while we do have some widespread leaks still, it used to be the case that e.g. some leaks would show up in 50-100 test files. Now the #1 leak by number of times it's seen in test files happens in just 12 filess, and there's a very long tail of leaks that only happen in one test somewhere. I.e. are specific to their own area. Just anecdotally I think it's had sort of the opposite problem recently, that it's getting good enough to start flagging about new leaks that really are specific to newly queued topics. E.g. [2] and [3] are two recent examples. But in both cases there seems to have been an understandable assumption that it was probably just "linux-leaks" being noisy, due to the job crying wolf in the past. 1. https://vm.nix.is/~avar/noindex/2022-12-03-git-leak-report.txt 2. https://lore.kernel.org/git/221108.864jv9sc9r.gmgdl@evledraar.gmail.com/ 3. https://lore.kernel.org/git/2488058d-dc59-e8c1-0611-fbcaeb083d73@web.de/
On Sat, Dec 03, 2022 at 02:41:04AM +0100, Ævar Arnfjörð Bjarmason wrote: > > I have similar feelings to you here. Back when cmake support was > > introduced, I explicitly wanted it to be something for people who cared > > about it, but that wouldn't bother people who didn't use it: > > > > https://lore.kernel.org/git/20200427200852.GC1728884@coredump.intra.peff.net/ > > > > I stand by that sentiment, but it seems to have crept up as a required > > thing to deal with, and that is mostly because of CI. Using cmake in CI > > is good for telling developers when a change they make has broken cmake. > > But it also makes cmake their problem, and not the folks interested in > > cmake. > > That's a bit of a pain, but I don't think the main problem is its > integration with CI. It's that there doesn't really seem to be an > interest in its active maintenance & review from its supposed main > target audience. Yeah, there are two issues: 1. People who don't care about cmake having to think about cmake at all. 2. People trying to fix cmake and not getting traction. My pain has usually been (1), but you were nice enough here to make it to (2). :) > > Now maybe attitudes have changed, and I am out of date, and cmake > > support is considered mature and really important (or maybe nobody even > > agreed with me back then ;) ). But if not, should we consider softening > > the CI output so that cmake failures aren't "real" failures? That seems > > drastic and mean, and I don't like it. But it's the root of the issue, > > IMHO. > > Yeah, maybe. Maybe if we broke it we'd get people showing up to maintain > it again :) By the way, I looked in the archive, and me complaining about cmake has come up once or twice. Johannes offered some helpful guidance on the value of running the vsbuild tests and how we might work around them: https://lore.kernel.org/git/nycvar.QRO.7.76.6.2008141352430.54@tvgsbejvaqbjf.bet/ > > As a side note, this isn't the only such instance of this problem. Two > > other things to think about: > > > > - You mentioned darwin fsmonitor code. And it's true that you can > > largely ignore it if you don't touch it. But every once in a while > > you get bit by it (e.g., enabling a new compiler warning which > > triggers in code you don't compile on your platform, and now you > > have to guess-and-check the fix with CI). This sucks, but is kind of > > inevitable on a cross-platform system. I think the issue with cmake > > is that because it's basically duplicating/wrapping the Makefile, it > > _feels_ unnecessary to people on platforms with working make, and > > triggers more frequently (because changes to the rest of the build > > system may break cmake in subtle ways). > > Yeah, we'll always have some cross-platform pain. > > But e.g. "chmod +x" just works in the Makefile, including when we run it > on Windows. And I've run it on Windows CI. But just upthread of here > Phillip is reporting that it doesn't work from the context of the CMake > recipe. > > I've been throwing some things at Windows CI, but I'm pretty stumped as > to what that might be. > > Some warning on Mac OS X is trivial by comparison :) That was just an example, of course. :) I have also done this kind of "guess and check" with the Windows CI. I think Johannes has given examples in the past of how to actually connect to a running CI instance and debug interactively, but I've never managed to remember the correct incantation at the moment I needed it. > > I'm not necessarily proposing to drop the leaks CI job here. I'm mostly > > philosophizing about the greater problem. In the early days of Git, the > > cross-platform testing philosophy was: somebody who cares will test on > > that platform and write a patch. If they don't, how important could it > > be? With CI that happens automatically and it becomes everybody's > > problem, which is a blessing and a curse. > > That's a definitely a bit of an irresistible digression :) > > First, I think we can agree that however frustrating that's been (and > sorry!) it would be a lot worse if my average response time to the leak > testing breaking something was measured in months :) Yes, to be clear, I have no problem with you in terms of responsiveness as maintaining the leaks system. It's mostly that I have to think of them _at all_ when working on something unrelated (that is not itself introducing new leaks). > I do think that whatever issues we've had with it (and in retrospect I'd > do some of it differently) that it's a lot more mature these days than > you might remember. Yeah, it definitely has been getting better, and I admit my opinion is based on a longer-term experience of a changing variable. There's probably some clever name for that kind of bias, but I don't know it offhand. -Peff
On Sat, Dec 03, 2022 at 02:41:04AM +0100, Ævar Arnfjörð Bjarmason wrote: > > I have similar feelings to you here. Back when cmake support was > > introduced, I explicitly wanted it to be something for people who cared > > about it, but that wouldn't bother people who didn't use it: > > > > https://lore.kernel.org/git/20200427200852.GC1728884@coredump.intra.peff.net/ > > > > I stand by that sentiment, but it seems to have crept up as a required > > thing to deal with, and that is mostly because of CI. Using cmake in CI > > is good for telling developers when a change they make has broken cmake. > > But it also makes cmake their problem, and not the folks interested in > > cmake. > > That's a bit of a pain, but I don't think the main problem is its > integration with CI. It's that there doesn't really seem to be an > interest in its active maintenance & review from its supposed main > target audience. > > Case in point this "ab/cmake-nix-and-ci" topic: It's been queued for > around 2 months now. I think CI *is* the problem here. The CMake bits are basically a black box to me (and I suspect a large number of other contributors, too). But when it breaks, the only reason we as a project end up noticing it is because it has fallout in CI. I would not be sad to make CI failures that are derived from CMake "soft" failures in the sense that they don't make the build red. But I think it's masking over a couple of bigger issues: - Why do we "support" two build systems in CI if one is supposed to only be here for those that care about it? IOW, even if we say that CMake support is nominally an opt-in thing, in reality it isn't because of the dependency via CI. - Why do we only *notice* these failures in CI? I found during my time as interim-maintainer the task of tracking down CI failures to quite frustrating. It is often quite difficult to reproduce CI failures locally (especially with exotic build and test configurations[^1]). It would be nice to be able to more easily see these failures locally before they hit CI. E.g., is it possible that I would work on a feature which somehow breaks the CMake build, and fail to notice it if I use "make" locally? Personally, I would not be sad to see CMake removed from the tree entirely because it has not seen enough maintenance and seems to be quite a headache. Thanks, Taylor [^1]: Not to mention non-Linux failures, though I think that is sort of par for the course if you're not using one of those platforms yourself.
On Mon, Dec 05 2022, Taylor Blau wrote: > On Sat, Dec 03, 2022 at 02:41:04AM +0100, Ævar Arnfjörð Bjarmason wrote: >> > I have similar feelings to you here. Back when cmake support was >> > introduced, I explicitly wanted it to be something for people who cared >> > about it, but that wouldn't bother people who didn't use it: >> > >> > https://lore.kernel.org/git/20200427200852.GC1728884@coredump.intra.peff.net/ >> > >> > I stand by that sentiment, but it seems to have crept up as a required >> > thing to deal with, and that is mostly because of CI. Using cmake in CI >> > is good for telling developers when a change they make has broken cmake. >> > But it also makes cmake their problem, and not the folks interested in >> > cmake. >> >> That's a bit of a pain, but I don't think the main problem is its >> integration with CI. It's that there doesn't really seem to be an >> interest in its active maintenance & review from its supposed main >> target audience. >> >> Case in point this "ab/cmake-nix-and-ci" topic: It's been queued for >> around 2 months now. > > I think CI *is* the problem here. The CMake bits are basically a black > box to me (and I suspect a large number of other contributors, too). But > when it breaks, the only reason we as a project end up noticing it is > because it has fallout in CI. > > I would not be sad to make CI failures that are derived from CMake > "soft" failures in the sense that they don't make the build red. But I > think it's masking over a couple of bigger issues: > > - Why do we "support" two build systems in CI if one is supposed to > only be here for those that care about it? IOW, even if we say that > CMake support is nominally an opt-in thing, in reality it isn't > because of the dependency via CI. I'm just trying to point out that that wouldn't be such a big deal if the cmake parts were being actively maintained. > - Why do we only *notice* these failures in CI? I found during my time > as interim-maintainer the task of tracking down CI failures to quite > frustrating. It is often quite difficult to reproduce CI failures > locally (especially with exotic build and test configurations[^1]). > > It would be nice to be able to more easily see these failures locally > before they hit CI. E.g., is it possible that I would work on a feature > which somehow breaks the CMake build, and fail to notice it if I use > "make" locally? Yes, some things will just work. E.g. it regex-parses out the Makefile to pick up the list of built-ins, so when we add a new one we'd usually not need to patch the cmake bits. Although that regex parsing is its own problem for some Makefile changes. To be fair the cases where we've needed to keep it in lockstep since it was added aren't that many. If you skim this and look for commits that alter both (or cmake quickly after the Makefile) you can spot them: git log --oneline --no-merges --full-diff --stat 1c966423263..origin/master -- contrib/buildsystems/CMakeLists.txt -- Makefile > Personally, I would not be sad to see CMake removed from the tree > entirely because it has not seen enough maintenance and seems to be > quite a headache. > > Thanks, > Taylor > > [^1]: Not to mention non-Linux failures, though I think that is sort of > par for the course if you're not using one of those platforms > yourself. I wouldn't mind either to see it gone, but when that was last discussed some people chimed in to say that it really made things easier on Windows, and I'm inclined to believe them. So I'm not trying to take their toys away, just changing it so that if you run into these cmake issues it'll be trivial to debug them, as you'll be able to test & run it outside of Windows. Now, I'm about to send out a v6 of it, which should address the last remaining Windows/VS issues. I'm not testing directly there, as I don't have such an installation, so it's possible I'm wrong. Maybe it works in CI, but still breaks for some reason when driven from VS. But I think if we can't get anyone who's running Windows+VS to be interested enough in this to test it the better thing is to just take this series. Maybe it breaks some subtle aspect of the VS integration, but then that'll be easier to fix on top than recovering from a "git rm" of it.
On Tue, Dec 06, 2022 at 12:46:50AM +0100, Ævar Arnfjörð Bjarmason wrote: > I'm just trying to point out that that wouldn't be such a big deal if > the cmake parts were being actively maintained. > > > - Why do we only *notice* these failures in CI? I found during my time > > as interim-maintainer the task of tracking down CI failures to quite > > frustrating. It is often quite difficult to reproduce CI failures > > locally (especially with exotic build and test configurations[^1]). > > > > It would be nice to be able to more easily see these failures locally > > before they hit CI. E.g., is it possible that I would work on a feature > > which somehow breaks the CMake build, and fail to notice it if I use > > "make" locally? > > Yes, some things will just work. E.g. it regex-parses out the Makefile > to pick up the list of built-ins, so when we add a new one we'd usually > not need to patch the cmake bits. Although that regex parsing is its own > problem for some Makefile changes. > > To be fair the cases where we've needed to keep it in lockstep since it > was added aren't that many. If you skim this and look for commits that > alter both (or cmake quickly after the Makefile) you can spot them: > > git log --oneline --no-merges --full-diff --stat 1c966423263..origin/master -- contrib/buildsystems/CMakeLists.txt -- Makefile I think that this is exactly the problem I'm trying to chime in about. We know that regular contributors can propose sensible changes to the Makefile. But integrating CMake into our tree in such a way that forces all Git developers to *also* be familiar with CMake is too large of a change to hoist onto the Git community, I think. > > Personally, I would not be sad to see CMake removed from the tree > > entirely because it has not seen enough maintenance and seems to be > > quite a headache. > > > > Thanks, > > Taylor > > > > [^1]: Not to mention non-Linux failures, though I think that is sort of > > par for the course if you're not using one of those platforms > > yourself. > > I wouldn't mind either to see it gone, but when that was last discussed > some people chimed in to say that it really made things easier on > Windows, and I'm inclined to believe them. > > So I'm not trying to take their toys away, just changing it so that if > you run into these cmake issues it'll be trivial to debug them, as > you'll be able to test & run it outside of Windows. OK. Personally, I wouldn't be sad to see it gone, either. I wonder: is there a way to keep it in our tree such that CMake breakage isn't everybody's problem (like Peff originally suggested when we were discussing this in the first place)? If so, I would vastly prefer that to the state that we have now. If not, I think moving it out of the tree is a sensible step, and one that I would advocate for. Thanks, Taylor
On Mon, Dec 05, 2022 at 06:34:09PM -0500, Taylor Blau wrote: > I think CI *is* the problem here. The CMake bits are basically a black > box to me (and I suspect a large number of other contributors, too). But > when it breaks, the only reason we as a project end up noticing it is > because it has fallout in CI. > > I would not be sad to make CI failures that are derived from CMake > "soft" failures in the sense that they don't make the build red. But I > think it's masking over a couple of bigger issues: > > - Why do we "support" two build systems in CI if one is supposed to > only be here for those that care about it? IOW, even if we say that > CMake support is nominally an opt-in thing, in reality it isn't > because of the dependency via CI. I think part of the reason cmake rose in importance via CI is that it's the de facto way to build for vscode. Before that CI job switched to cmake, there was some other alternate build system (vcxproj). So two things I'd consider here: - how important is it for us to do the vscode build as part of regular CI (as opposed to folks who are interested in it running it themselves). Dscho gave some real data in the thread I linked to earlier (which indicates that yes, it helps, but not that often). - what's the status of cmake versus vcxproj? My impression (though I admit based on my half-paying-attention-to of the topic) is that cmake should replace vcxproj, and nobody would ever want to work on vcxproj anymore. But if that's not right, then does vcxproj cause headaches for non-Windows devs less often? I don't really remember dealing with it much, but I may have just been lucky. > - Why do we only *notice* these failures in CI? I found during my time > as interim-maintainer the task of tracking down CI failures to quite > frustrating. It is often quite difficult to reproduce CI failures > locally (especially with exotic build and test configurations[^1]). I think that's just the normal platform issues. You don't use it on your system, so you don't notice until it runs in CI. If people don't run CI themselves, then it falls to the maintainer's CI run, which is a pain for them. But that's as true of other operating systems, exotic test flags, etc, as it is of cmake. > It would be nice to be able to more easily see these failures locally > before they hit CI. E.g., is it possible that I would work on a feature > which somehow breaks the CMake build, and fail to notice it if I use > "make" locally? That seems like going in the opposite direction from what you're saying above: doubling down that if cmake is broken by a change, it is the responsibility of the dev who made the change to find and fix it. I do like that Ævar is trying to make it easier to run cmake from Linux in order to find that without using CI. But that does seem orthogonal to me to the notion of "who is responsible for finding and fixing cmake problems". To me, that decision is really rooted in "is cmake something the Git project supports, or is it a side-thing that some folks volunteer to keep working?". > Personally, I would not be sad to see CMake removed from the tree > entirely because it has not seen enough maintenance and seems to be > quite a headache. I don't mind having it in-tree if I can ignore it (assuming the project attitude is the "it's a side thing" from above). It's the CI failures that make it hard to ignore. -Peff
On Mon, Dec 05 2022, Jeff King wrote: > On Mon, Dec 05, 2022 at 06:34:09PM -0500, Taylor Blau wrote: > >> I think CI *is* the problem here. The CMake bits are basically a black >> box to me (and I suspect a large number of other contributors, too). But >> when it breaks, the only reason we as a project end up noticing it is >> because it has fallout in CI. >> >> I would not be sad to make CI failures that are derived from CMake >> "soft" failures in the sense that they don't make the build red. But I >> think it's masking over a couple of bigger issues: >> >> - Why do we "support" two build systems in CI if one is supposed to >> only be here for those that care about it? IOW, even if we say that >> CMake support is nominally an opt-in thing, in reality it isn't >> because of the dependency via CI. > > I think part of the reason cmake rose in importance via CI is that it's > the de facto way to build for vscode. Before that CI job switched to > cmake, there was some other alternate build system (vcxproj). > > So two things I'd consider here: > > - how important is it for us to do the vscode build as part of regular > CI (as opposed to folks who are interested in it running it > themselves). Dscho gave some real data in the thread I linked to > earlier (which indicates that yes, it helps, but not that often). > > - what's the status of cmake versus vcxproj? My impression (though I > admit based on my half-paying-attention-to of the topic) is that > cmake should replace vcxproj, and nobody would ever want to work on I think the intent was to deprecate vcxproj, but I'm not sure, and I wonder if the "cmake" is the proposed path forward why we still have it in-tree anymore. > vcxproj anymore. But if that's not right, then does vcxproj cause > headaches for non-Windows devs less often? I don't really remember > dealing with it much, but I may have just been lucky. It was less painful for non-Windows folks, but I understand the cmake integration was also much nicer for VS. I.e. it's picked up by the IDE in a way that the "make" shim wasn't. > [...] > That seems like going in the opposite direction from what you're saying > above: doubling down that if cmake is broken by a change, it is the > responsibility of the dev who made the change to find and fix it. > > I do like that Ævar is trying to make it easier to run cmake from Linux > in order to find that without using CI. But that does seem orthogonal to > me to the notion of "who is responsible for finding and fixing cmake > problems". To me, that decision is really rooted in "is cmake something > the Git project supports, or is it a side-thing that some folks > volunteer to keep working?". I agree with that... >> Personally, I would not be sad to see CMake removed from the tree >> entirely because it has not seen enough maintenance and seems to be >> quite a headache. > > I don't mind having it in-tree if I can ignore it (assuming the project > attitude is the "it's a side thing" from above). It's the CI failures > that make it hard to ignore. ...but on this thread-at-large, I'd much rather see us focus on just reviewing the patches I have here than raising the burden of proof to whether we should get rid of it entirely. If we make the CI failures "soft" failures or move it out-of-tree entirely it would still be useful to be able to run the cmake recipe on *nix.
On Tue, Dec 06, 2022 at 02:43:17AM +0100, Ævar Arnfjörð Bjarmason wrote: > > I don't mind having it in-tree if I can ignore it (assuming the project > > attitude is the "it's a side thing" from above). It's the CI failures > > that make it hard to ignore. > > ...but on this thread-at-large, I'd much rather see us focus on just > reviewing the patches I have here than raising the burden of proof to > whether we should get rid of it entirely. Fair. In case it is not obvious, I have no interest in reviewing cmake patches. ;) But I will at least stop making noise in the thread. > If we make the CI failures "soft" failures or move it out-of-tree > entirely it would still be useful to be able to run the cmake recipe on > *nix. Agreed. -Peff
On Mon, Dec 05 2022, Jeff King wrote: > On Tue, Dec 06, 2022 at 02:43:17AM +0100, Ævar Arnfjörð Bjarmason wrote: > >> > I don't mind having it in-tree if I can ignore it (assuming the project >> > attitude is the "it's a side thing" from above). It's the CI failures >> > that make it hard to ignore. >> >> ...but on this thread-at-large, I'd much rather see us focus on just >> reviewing the patches I have here than raising the burden of proof to >> whether we should get rid of it entirely. > > Fair. In case it is not obvious, I have no interest in reviewing cmake > patches. ;) But I will at least stop making noise in the thread. I'm fine with the running commentary on the future direction, I think it's also very useful. I just wanted to also note the need to keep the eyes on the ball a bit :) >> If we make the CI failures "soft" failures or move it out-of-tree >> entirely it would still be useful to be able to run the cmake recipe on >> *nix. > > Agreed. Just to add my own digression: I asked in some past thread (which I'm too lazy to dig up) why it was the cmake file couldn't just dispatch to "make" for most things. I.e. it needs to at some level be aware of what it's building for the IDE integration, but for say making a "grep.o" there's no reason it couldn't be running: make grep.o Instead of: cc <args> -o grep grep.c [...] which requires duplicating much of the Makefile logic (possibly with some Makefile shim to not consider any dependencies in that case). Even if we couldn't do that for *.c code for some reason it could do it e.g. creating the generated *.h files, which is logic we currentnly duplicate. The "win+VS build" job even has a hard dependency on GNU make currently, in needing to run "make artifacts-tar" to get to the "win+VS test" stage. But apparently the reason for *that* is that another goal of the integration was to avoid having to have GNU make installed at all, which comes in a different package than the one that would ship VS+cmake (or something?). Which might be something to re-visit, i.e. maybe we could eventually say "yes, you can have VS+cmake, but it's not too much to ask that you install GNU make as a one-off". Doing that would then reduce the duplication to the point where the cmake recipe would be a thin shim around the Makefile. I don't use this development setup, but if the CI job is managing to download and run GNU make it can't be that hard for an end-user to similarly install it (but what do I know?).
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > Just to add my own digression: I asked in some past thread (which I'm > too lazy to dig up) why it was the cmake file couldn't just dispatch to > "make" for most things. > > I.e. it needs to at some level be aware of what it's building for the > IDE integration, but for say making a "grep.o" there's no reason it > couldn't be running: > > make grep.o > > Instead of: > > cc <args> -o grep grep.c [...] > > which requires duplicating much of the Makefile logic (possibly with > some Makefile shim to not consider any dependencies in that case). That leads to a question at the other extreme. Why does any logic in CMakeLists.txt even have to exist at all? Whenever it is asked to make foo, it can be running "make foo" instead of having its own logic at all. ;-)
On 06/12/2022 03:52, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > >> Just to add my own digression: I asked in some past thread (which I'm >> too lazy to dig up) why it was the cmake file couldn't just dispatch to >> "make" for most things. Because make is not installed by default on Windows. Our CI job uses msbuild (whatever that is) and when I was playing with Visual Studio last week it was using ninja. >> I.e. it needs to at some level be aware of what it's building for the >> IDE integration, but for say making a "grep.o" there's no reason it >> couldn't be running: >> >> make grep.o >> >> Instead of: >> >> cc <args> -o grep grep.c [...] >> >> which requires duplicating much of the Makefile logic (possibly with >> some Makefile shim to not consider any dependencies in that case). > > That leads to a question at the other extreme. Why does any logic > in CMakeLists.txt even have to exist at all? Whenever it is asked > to make foo, it can be running "make foo" instead of having its own > logic at all. ;-) Yes, if make was available then we wouldn't need to use CMake. Best Wishes Phillip
On Tue, Dec 06 2022, Phillip Wood wrote: > On 06/12/2022 03:52, Junio C Hamano wrote: >> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: >> >>> Just to add my own digression: I asked in some past thread (which I'm >>> too lazy to dig up) why it was the cmake file couldn't just dispatch to >>> "make" for most things. > > Because make is not installed by default on Windows. Our CI job uses > msbuild (whatever that is) and when I was playing with Visual Studio > last week it was using ninja. > >>> I.e. it needs to at some level be aware of what it's building for the >>> IDE integration, but for say making a "grep.o" there's no reason it >>> couldn't be running: >>> >>> make grep.o >>> >>> Instead of: >>> >>> cc <args> -o grep grep.c [...] >>> >>> which requires duplicating much of the Makefile logic (possibly with >>> some Makefile shim to not consider any dependencies in that case). >> That leads to a question at the other extreme. Why does any logic >> in CMakeLists.txt even have to exist at all? Whenever it is asked >> to make foo, it can be running "make foo" instead of having its own >> logic at all. ;-) > > Yes, if make was available then we wouldn't need to use CMake. I think Junio and I are talking about something slightly different. Yes "make" isn't available by default. Getting it requires installing a larger SDK. But if you look at the history of contrib/vscode/README.md in our tree you'll see that we used to support this "Visual Studio Solution" for years via GNU make, it probably still works. The change in 4c2c38e800f (ci: modification of main.yml to use cmake for vs-build job, 2020-06-26) shows when the CI was switched over to using cmake instead. The code to support that is still in-tree as the "vcxproj" target in "config.mak.uname", which calls out to the ~1k lines of Perl code in contrib/buildsystems/Generators/*. I'm not suggesting we go back to that. The question is whether the trade-off of supporting an entirely separate build system without a GNU make dependency was worth it. On the one hand those developing on Windows don't need to install it as a package, on the other hand we end up spending more developer time in writing duplicate build logic. The advantage of cmake is that it knows how to generate all that VS-integration XML etc., so as soon as it knows how to build Git you get that for free. But I think you can get that while not having a "real" cmake recipe, but just a thin shim for calling out to the Makefile. Is such a thing a hack? Yes. Is it silly to e.g. build with "ninja" and really have it just shell out to "make"? Yes. But it might still be worth it if we judge that the goal of getting that VS integration is sufficient. And that we're not willing to absorb the cost of maintaining two distinct build recipes in perpetuity.
On Mon, Dec 05, 2022 at 09:05:03PM -0500, Jeff King wrote: > On Tue, Dec 06, 2022 at 02:43:17AM +0100, Ævar Arnfjörð Bjarmason wrote: > > > > I don't mind having it in-tree if I can ignore it (assuming the project > > > attitude is the "it's a side thing" from above). It's the CI failures > > > that make it hard to ignore. > > > > ...but on this thread-at-large, I'd much rather see us focus on just > > reviewing the patches I have here than raising the burden of proof to > > whether we should get rid of it entirely. > > Fair. In case it is not obvious, I have no interest in reviewing cmake > patches. ;) But I will at least stop making noise in the thread. Same, and same. Sorry for semi-derailing this thread. I don't feel strongly enough about the CMake stuff to start a new thread about it on the list, but while others are discussing it, I figured that I might as well chime in. Thanks Taylor
Hi, On Tue, 6 Dec 2022, Ævar Arnfjörð Bjarmason wrote: > On Tue, Dec 06 2022, Phillip Wood wrote: > > > On 06/12/2022 03:52, Junio C Hamano wrote: > >> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > >> > >>> Just to add my own digression: I asked in some past thread (which > >>> I'm too lazy to dig up) why it was the cmake file couldn't just > >>> dispatch to "make" for most things. > > > > Because make is not installed by default on Windows. Our CI job uses > > msbuild (whatever that is) and when I was playing with Visual Studio > > last week it was using ninja. > > > >>> I.e. it needs to at some level be aware of what it's building for the > >>> IDE integration, but for say making a "grep.o" there's no reason it > >>> couldn't be running: > >>> > >>> make grep.o > >>> > >>> Instead of: > >>> > >>> cc <args> -o grep grep.c [...] > >>> > >>> which requires duplicating much of the Makefile logic (possibly with > >>> some Makefile shim to not consider any dependencies in that case). > >> That leads to a question at the other extreme. Why does any logic > >> in CMakeLists.txt even have to exist at all? Whenever it is asked > >> to make foo, it can be running "make foo" instead of having its own > >> logic at all. ;-) > > > > Yes, if make was available then we wouldn't need to use CMake. > > I think Junio and I are talking about something slightly different. Yes > "make" isn't available by default. Getting it requires installing a > larger SDK. > > But if you look at the history of contrib/vscode/README.md in our tree > you'll see that we used to support this "Visual Studio Solution" for > years via GNU make, it probably still works. It probably doesn't. Last time I had to use it, during the embargoed v2.37.1 release process, it didn't. I had to add plenty of patches to make it work again: https://github.com/git-for-windows/git/compare/323a69709944%5E...323a69709944%5E2 > The change in 4c2c38e800f (ci: modification of main.yml to use cmake for > vs-build job, 2020-06-26) shows when the CI was switched over to using > cmake instead. > > The code to support that is still in-tree as the "vcxproj" target in > "config.mak.uname", which calls out to the ~1k lines of Perl code in > contrib/buildsystems/Generators/*. At some stage we can probably get rid of the `vcxproj` code. Before that, we can even get rid of the `vcproj` code that is bit-rotting in `contrib/buildsystems/`. But there seems no harm, and less maintenance burden, in keeping the `vcxproj`/`vcproj` parts where they are, as they are. Taking a step back, I see that we got far away from the topic that started this thread. So here's my take on `ab/cmake-nix-and-ci`: While that patch series' intention is apparently to make it easier to diagnose and fix CI problems, I only see that it adds new problems. It won't make it possible to diagnose most win+VS problems because they don't reproduce on Linux. But the patches already did introduce Windows-specific problems merely by trying to get the Linux side of CMake to work. And trying to keep CMake working both on Linux and on Windows would cause many more problems in the future. And we do not even need CMake support for Linux, `make` works well there already. It would increase the maintenance burden unnecessarily. I am therefore suggesting to drop `cmake-nix-and-ci` entirely. To address the concern about broken CI runs, I hoped that monitoring them and helping contributors by suggesting fixups was working well enough. It used to be okay for patches to be contributed that caused CI to fail, we simply worked together to fix CI, as a team, and that was that. We didn't "blame" the contributors, or anything, when CI runs failed because of their patches. After all, possible causes of CI failures include that patches might be applied on top of other commits than intended, or that patch series interact in unfortunate ways. Junio, maybe you could clarify your take on this? As project lead, it is your decision to define how Git uses Continuous Builds, and how the project handles failed CI runs. Ciao, Johannes
On Thu, Dec 08 2022, Johannes Schindelin wrote: > Hi, > > On Tue, 6 Dec 2022, Ævar Arnfjörð Bjarmason wrote: > >> On Tue, Dec 06 2022, Phillip Wood wrote: >> >> > On 06/12/2022 03:52, Junio C Hamano wrote: >> >> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: >> >> >> >>> Just to add my own digression: I asked in some past thread (which >> >>> I'm too lazy to dig up) why it was the cmake file couldn't just >> >>> dispatch to "make" for most things. >> > >> > Because make is not installed by default on Windows. Our CI job uses >> > msbuild (whatever that is) and when I was playing with Visual Studio >> > last week it was using ninja. >> > >> >>> I.e. it needs to at some level be aware of what it's building for the >> >>> IDE integration, but for say making a "grep.o" there's no reason it >> >>> couldn't be running: >> >>> >> >>> make grep.o >> >>> >> >>> Instead of: >> >>> >> >>> cc <args> -o grep grep.c [...] >> >>> >> >>> which requires duplicating much of the Makefile logic (possibly with >> >>> some Makefile shim to not consider any dependencies in that case). >> >> That leads to a question at the other extreme. Why does any logic >> >> in CMakeLists.txt even have to exist at all? Whenever it is asked >> >> to make foo, it can be running "make foo" instead of having its own >> >> logic at all. ;-) >> > >> > Yes, if make was available then we wouldn't need to use CMake. >> >> I think Junio and I are talking about something slightly different. Yes >> "make" isn't available by default. Getting it requires installing a >> larger SDK. >> >> But if you look at the history of contrib/vscode/README.md in our tree >> you'll see that we used to support this "Visual Studio Solution" for >> years via GNU make, it probably still works. > > It probably doesn't. Last time I had to use it, during the embargoed > v2.37.1 release process, it didn't. I had to add plenty of patches to make > it work again: > https://github.com/git-for-windows/git/compare/323a69709944%5E...323a69709944%5E2 > >> The change in 4c2c38e800f (ci: modification of main.yml to use cmake for >> vs-build job, 2020-06-26) shows when the CI was switched over to using >> cmake instead. >> >> The code to support that is still in-tree as the "vcxproj" target in >> "config.mak.uname", which calls out to the ~1k lines of Perl code in >> contrib/buildsystems/Generators/*. > > At some stage we can probably get rid of the `vcxproj` code. Before that, > we can even get rid of the `vcproj` code that is bit-rotting in > `contrib/buildsystems/`. But there seems no harm, and less maintenance > burden, in keeping the `vcxproj`/`vcproj` parts where they are, as they > are. > > Taking a step back, I see that we got far away from the topic that started > this thread. > > So here's my take on `ab/cmake-nix-and-ci`: While that patch series' > intention is apparently to make it easier to diagnose and fix CI problems, > I only see that it adds new problems. It won't make it possible to > diagnose most win+VS problems because they don't reproduce on Linux. That would also be my take if that was the goal of the series. I agree that would be pretty pointless. Why test win+VS-specific code on Linux? That makes no sense. But that's not the goal. It's to make it easier to test the majority of the platform-agnostic code in the cmake recipe. E.g. here's some past commits from myself, Jeff King and Han-Wen (who I'm pretty sure doesn't use Windows) where we've had to patch the cmake recipe in addition to the Makefile: ef8a6c62687 (reftable: utility functions, 2021-10-07) cfe853e66be (hook-list.h: add a generated list of hooks, like config-list.h, 2021-09-26) d7a5649c82d (make git-bugreport a builtin, 2020-08-13) b5dd96b70ac (make credential helpers builtins, 2020-08-13) Doing that currently requires bouncing things off the Windows CI. With ab/cmake-nix-and-ci you can not only build (which currently works) but run the full test against the cmake build on *nix in minutes. That's a big improvement. I think this also misrepresents the nature of the cmake recipe, and how much of it is truly MSVC or VS-specific. I count less than 20 lines in a ~1.1k line recipe that are really "MSVC"-specific. I.e. guarded by "if" branches checking "MSVC" and 'CMAKE_C_COMPILER_ID STREQUAL "MSVC"'. The rest are general in nature. E.g. you can run the tests with "ctest" from the VS GUI. That's not because there's a VS-specific "hey Visual Studio, here's our tests" part of the recipe. Rather there's a generic cross-platform method of declaring how to run the tests, which cmake itself then knows to pick up and generate a VS-specific asset with. Whereas you seem to be suggesting that the recipe is so Windows+VS-specific that testing it on other platforms isn't going to tell you much. I don't think that's true. > But the patches already did introduce Windows-specific problems merely > by trying to get the Linux side of CMake to work. This seems like vague commentary on past bugs. Do you have any specific concerns about things that are broken by the current v6 iteration? > And trying to keep CMake > working both on Linux and on Windows would cause many more problems in the > future. And we do not even need CMake support for Linux, `make` works well > there already. It would increase the maintenance burden unnecessarily. If you're going to argue that "we do not even need CMake support for Linux" you're not making an argument against ab/cmake-nix-and-ci, but against the status quo on "master". It's already the case that it mostly "works" on Linux, that's been the case since day 1. E.g. Yuyi's a561962479c (cmake: fix CMakeLists.txt on Linux, 2022-05-24) earlier this year. > I am therefore suggesting to drop `cmake-nix-and-ci` entirely. I wouldn't mind if we declare that it should never work on Linux, and I wouldn't mind if we drop ab/cmake-nix-and-ci entirely. But only *if* the cmake recipe becomes purely a "lag-behind" alternative build system that the Windows folks are responsible for updating, or to submit follow-up patches for if it breaks. That's not the status quo now, as e.g. Jeff King summarized nicely here: https://lore.kernel.org/git/Y4qF3iHW2s+I0yNe@coredump.intra.peff.net/ You don't seem to be suggesting a productive way forward with that. I don't think dropping it, or even making the Windows cmake CI optional would be a productive way forward, I think it would ultimately waste more of your time, and that of other Windows developers. But I don't see how that isn't the logical conclusion to not providing specific feedback on this topic & finding a way forward with it.
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > Junio, maybe you could clarify your take on this? As project lead, it is > your decision to define how Git uses Continuous Builds, and how the > project handles failed CI runs. I have pretty much been with what Peff and Taylor said in the thread already ever since we added CMake support to help Windows/VS folks. I agree with you that we do not need to run it for Linux or macOS, and if the promised/hoped-for benefit, i.e. that running them on non-Windows build would uncover issues that are common across the platform and help Windows, is not something that is likely to materialize, I'd prefer to see our resources (CI time and developer attention) not spent on that. I do not think "how the project handles filed CI runs" is a very big issue. I often ignore partial failures (e.g. "winVS(n) test job triggered rate limit") and the only annoyance I feel is that such a temporary failure contribute one more message to my trash mailbox, and I can learn to do the same for a test that marked as failed due to linux-cmake-ctest job. I expect that regular contributors are doing the same pretty much. How blocking is a CI failure for drive-by contributors who use GGG? While I do not necessarily value drive-by contributions as much as you do, if such "an unimportant failure we can ignore" discourages those coming from GGG route, that would be unfortunate, exactly because they may not have contributed anything to the failures. This is not just cmake-ctest, but the leak checking job where a new use of a tool that is known to be leaky in a test can turn a test that has been passing to fail. If we can mark failures in selected jobs as non-blocking, we definitely should do so. Between keeping and marking linux-cmake-ctest as non-blocking, and removing it altogether, I am inclined to say that I'd favor the latter for the reasons I explained earlier in this message. But to help casual contributors coming via GGG, we would anyway need to (1) allow submitting even with failing tests, and (2) tell them that it is OK to do so. Which means it is not the end of the world, from the point of view of helping casual developers, if we had kept these brittle CI jobs like linux-cmake-ctest and linux-leaks.
On Fri, Dec 09 2022, Junio C Hamano wrote: > Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > >> Junio, maybe you could clarify your take on this? As project lead, it is >> your decision to define how Git uses Continuous Builds, and how the >> project handles failed CI runs. > > I have pretty much been with what Peff and Taylor said in the thread > already ever since we added CMake support to help Windows/VS folks. > I agree with you that we do not need to run it for Linux or macOS, > and if the promised/hoped-for benefit, i.e. that running them on > non-Windows build would uncover issues that are common across the > platform and help Windows, is not something that is likely to > materialize, I'd prefer to see our resources (CI time and developer > attention) not spent on that. I think this should be addressed by the "I count less than 20 lines in a ~1.1k line recipe that are really "MSVC"-specific" in the sibling mail[1]. I.e. the large majority of it is generic recipe code that's run on all platforms. > I do not think "how the project handles filed CI runs" is a very big > issue. I often ignore partial failures (e.g. "winVS(n) test job > triggered rate limit") and the only annoyance I feel is that such a > temporary failure contribute one more message to my trash mailbox, > and I can learn to do the same for a test that marked as failed due > to linux-cmake-ctest job. I expect that regular contributors are > doing the same pretty much. > > How blocking is a CI failure for drive-by contributors who use GGG? > While I do not necessarily value drive-by contributions as much as > you do, if such "an unimportant failure we can ignore" discourages > those coming from GGG route, that would be unfortunate, exactly > because they may not have contributed anything to the failures. > This is not just cmake-ctest, but the leak checking job where a new > use of a tool that is known to be leaky in a test can turn a test > that has been passing to fail. If we can mark failures in selected > jobs as non-blocking, we definitely should do so. I realize that we've been digressing to the larger topic of what to do with CI in general, but I don't think the question of whether say "win+VS build" should "soft fail" is something we should conflate with this "ab/cmake-nix-and-ci" topic. It doesn't change the status quo there, and I think is a net improvement. Even if we make the CI for anything cmake-related soft-fail, this topic will still help to get it back up to speed, as you'll be able to run the full cmake+ctest chain on non-Windows. > Between keeping and marking linux-cmake-ctest as non-blocking, and > removing it altogether, I am inclined to say that I'd favor the > latter for the reasons I explained earlier in this message. But to > help casual contributors coming via GGG, we would anyway need to (1) > allow submitting even with failing tests, and (2) tell them that it > is OK to do so. Which means it is not the end of the world, from > the point of view of helping casual developers, if we had kept these > brittle CI jobs like linux-cmake-ctest and linux-leaks. I can peel off the commit that adds the "linux-cmake-ctest" CI job from this series, or even just make it do the equivalent of: cmake && make || echo oops, we're broken So it'll "soft-fail" (AFAIK GitHub CI, unlike GitLab[2] doesn't support a native way to "soft-fail"). But I don't think that doing that would help without *also* making "win+VS {build,test}" soft-fail. I.e. if "linux-cmake-ctest" *and* "win+VS" (with all else passing) you can be pretty sure it's a generic cmake problem. If only one or the other is failing somewhere in cmake having the "linux-cmake-ctest" job now will help narrow down whether it's a platform-specific cmake issue. So, just let me know what you'd prefer, but I think per the above even if you're impatient with cmake failures the "linux-cmake-ctest" job should help spend less time on them. 1. https://lore.kernel.org/git/221208.86wn726qcv.gmgdl@evledraar.gmail.com/ 2. https://docs.gitlab.com/ee/ci/yaml/#allow_failure -- In the UX: green=passing, red=failing, yellow=soft-fail
diff --git a/t/test-lib.sh b/t/test-lib.sh index ce319c9963e..9c63ee428f2 100644 --- a/t/test-lib.sh +++ b/t/test-lib.sh @@ -57,9 +57,11 @@ if test -n "$GIT_TEST_BUILD_DIR" then GIT_BUILD_DIR="$GIT_TEST_BUILD_DIR" elif ! test -x "$GIT_BUILD_DIR/git" && - test -x "$GIT_BUILD_DIR/contrib/buildsystems/out/git" + test -d "$GIT_BUILD_DIR/contrib/buildsystems/out" then - GIT_BUILD_DIR="$GIT_SOURCE_DIR/contrib/buildsystems/out" + GIT_BUILD_OPTIONS="$(find "$GIT_BUILD_DIR/contrib/buildsystems/out" \ + -type f -name 'GIT-BUILD-OPTIONS')" + GIT_BUILD_DIR="${GIT_BUILD_OPTIONS%/GIT-BUILD-OPTIONS}" GIT_AUTO_CONTRIB_BUILDSYSTEMS_OUT=t # On Windows, we must convert Windows paths lest they contain a colon @@ -1646,7 +1648,7 @@ remove_trash_directory "$TRASH_DIRECTORY" || { # anything using lib-subtest.sh if test -n "$GIT_AUTO_CONTRIB_BUILDSYSTEMS_OUT" && test -t 1 then - say "setup: had no ../git, but found & used cmake built git in ../contrib/buildsystems/out/git" + say "setup: had no ../git, but found & used cmake built git in '$GIT_BUILD_DIR/git'" fi remove_trash=t