| Message ID | pull.970.git.1622828605.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Make CMake work out of the box | expand |
Hi, On 05/06/21 00.43, Matthew Rogers via GitGitGadget wrote: > This pull request comes from our discussion here[1], and I think these > patches provide a good compromise around the concerns discussed there > > 1: > https://lore.kernel.org/git/CAOjrSZusMSvs7AS-ZDsV8aQUgsF2ZA754vSDjgFKMRgi_oZAWw@mail.gmail.com/ > > CCing the people involved in the original discussion. This focused on improving CMake support, especially on Visual Studio, right? Then so we have three ways to build Git: 1. plain Makefile 2. ./configure (really just wrapper on top of Makefile) 3. generate build file with CMake If we want to support all of them, it may makes sense to have CI jobs that perform build with each options above.
On Fri, Jun 4, 2021 at 11:40 PM Bagas Sanjaya <bagasdotme@gmail.com> wrote: > > Hi, > > On 05/06/21 00.43, Matthew Rogers via GitGitGadget wrote: > > This pull request comes from our discussion here[1], and I think these > > patches provide a good compromise around the concerns discussed there > > > > 1: > > https://lore.kernel.org/git/CAOjrSZusMSvs7AS-ZDsV8aQUgsF2ZA754vSDjgFKMRgi_oZAWw@mail.gmail.com/ > > > > CCing the people involved in the original discussion. > > This focused on improving CMake support, especially on Visual Studio, right? > > Then so we have three ways to build Git: > 1. plain Makefile > 2. ./configure (really just wrapper on top of Makefile) > 3. generate build file with CMake > > If we want to support all of them, it may makes sense to have CI jobs > that perform build with each options above. > > -- > An old man doll... just what I always wanted! - Clara Here's my understanding of the current pipeline situation: I know the Visual Studio CMake generator is currently used to build on Windows for gitgitgadget[1]. I'm not sure how worth it it would be to add another pipeline just to test if we correctly set EXPORT_COMPILE_COMMANDS=TRUE on by default correctly. I think adding support for running cmake builds on linux is a bit of a waste since those platforms should have ready access to make, and that's the build method that gets the official support. I don't really have much more of a position on this other than "Probably not worth it to add a cmake build on linux" 1: https://github.com/gitgitgadget/git/runs/2748313673 -- Matthew Rogers
Hi, On Sat, 5 Jun 2021, Bagas Sanjaya wrote: > On 05/06/21 00.43, Matthew Rogers via GitGitGadget wrote: > > This pull request comes from our discussion here[1], and I think these > > patches provide a good compromise around the concerns discussed there > > > > 1: > > https://lore.kernel.org/git/CAOjrSZusMSvs7AS-ZDsV8aQUgsF2ZA754vSDjgFKMRgi_oZAWw@mail.gmail.com/ > > > > CCing the people involved in the original discussion. > > This focused on improving CMake support, especially on Visual Studio, right? > > Then so we have three ways to build Git: > 1. plain Makefile > 2. ./configure (really just wrapper on top of Makefile) > 3. generate build file with CMake > > If we want to support all of them, it may makes sense to have CI jobs that > perform build with each options above. We already exercise the plain Makefile plenty, and the CMake-based build using Windows (in the `vs-build` job in `.github/workflows/main.yml`). I do not see that it is worth spending many electrons exercising the `./configure` way, seeing as the preferred way to build Git is by using the `Makefile` directly. And our CMake configuration only really works on Windows, the attempts to get it to work on Linux were met with less enthusiasm, seeing as the `Makefile` approach is the recommended (and supported) one. tl;dr I don't think we need to augment our CI jobs as suggested. Ciao, Dscho
On 10/06/2021 10:43, Johannes Schindelin wrote: > Hi, > > On Sat, 5 Jun 2021, Bagas Sanjaya wrote: > >> On 05/06/21 00.43, Matthew Rogers via GitGitGadget wrote: >>> This pull request comes from our discussion here[1], and I think these >>> patches provide a good compromise around the concerns discussed there >>> >>> 1: >>> https://lore.kernel.org/git/CAOjrSZusMSvs7AS-ZDsV8aQUgsF2ZA754vSDjgFKMRgi_oZAWw@mail.gmail.com/ >>> >>> CCing the people involved in the original discussion. Matt, Thanks for picking this up and the approach to working around the updated build approach of recent Visual Studio versions. It looks good to me, but the CI should also be tweaked (see below) so that it is tested. >> This focused on improving CMake support, especially on Visual Studio, right? >> >> Then so we have three ways to build Git: >> 1. plain Makefile >> 2. ./configure (really just wrapper on top of Makefile) >> 3. generate build file with CMake >> >> If we want to support all of them, it may makes sense to have CI jobs that >> perform build with each options above. > We already exercise the plain Makefile plenty, and the CMake-based build > using Windows (in the `vs-build` job in `.github/workflows/main.yml`). There is one 'gotcha' in the yml (probably historical) in that it doesn't actually test the approach/changes that Matt addresses regarding my [1]. That is, I'm looking at the 'out of the box' view, while the yml test _preloads_ the vcpkg artefacts. There is also the (on Windows) issue that the ARM support has recently been developed which also fudges the CmakeLists.txt file but forgot about the assumption in the vcpkg install batch file that the default is the x86 setup. > > I do not see that it is worth spending many electrons exercising the > `./configure` way, seeing as the preferred way to build Git is by using > the `Makefile` directly. > > And our CMake configuration only really works on Windows, the attempts to > get it to work on Linux were met with less enthusiasm, seeing as the > `Makefile` approach is the recommended (and supported) one. > > tl;dr I don't think we need to augment our CI jobs as suggested. I'd agree that there's no need to augment the CI job to expressly check the other flags, but the existing test should reflect the intent of the patches (i.e. no preloading of the vcpkg artefacts). I haven't had much time to catch up on Git, and I'm off-line again from Sat night for another week.
Hi Philip, On Fri, 18 Jun 2021, Philip Oakley wrote: > On 10/06/2021 10:43, Johannes Schindelin wrote: > > > > On Sat, 5 Jun 2021, Bagas Sanjaya wrote: > > > >> This focused on improving CMake support, especially on Visual Studio, right? > >> > >> Then so we have three ways to build Git: > >> 1. plain Makefile > >> 2. ./configure (really just wrapper on top of Makefile) > >> 3. generate build file with CMake > >> > >> If we want to support all of them, it may makes sense to have CI jobs that > >> perform build with each options above. > > > > We already exercise the plain Makefile plenty, and the CMake-based build > > using Windows (in the `vs-build` job in `.github/workflows/main.yml`). > > There is one 'gotcha' in the yml (probably historical) in that it > doesn't actually test the approach/changes that Matt addresses regarding > my [1]. > > That is, I'm looking at the 'out of the box' view, while the yml test > _preloads_ the vcpkg artefacts. We need to "pre-load" them because building them would add another whopping 20 minutes to each CI run. And I am not talking total time, but wall-clock time. And we're not in the business of testing vcpkg's build. So I am really not in favor of even thinking about changing this "pre-loading" strategy. Ciao, Dscho
Hi Dscho On 18/06/2021 14:42, Johannes Schindelin wrote: >>> We already exercise the plain Makefile plenty, and the CMake-based build >>> using Windows (in the `vs-build` job in `.github/workflows/main.yml`). >> There is one 'gotcha' in the yml (probably historical) in that it >> doesn't actually test the approach/changes that Matt addresses regarding >> my [1]. >> >> That is, I'm looking at the 'out of the box' view, while the yml test >> _preloads_ the vcpkg artefacts. > We need to "pre-load" them because building them would add another > whopping 20 minutes to each CI run. And I am not talking total time, but > wall-clock time. > > And we're not in the business of testing vcpkg's build. > > So I am really not in favor of even thinking about changing this > "pre-loading" strategy. > > I can see the common sense in that, however I was trying to highlight that the approach in patch series could go stale, as did the previous method. Making the entry ramp to investigating the code for the wide variety windows users should have _some_ testing.. I don't have any good ideas about how to get out of that 20 minute Catch-22 issue at the moment. Maybe it needs an independent, on-demand (i.e. infrequent;-) test. Maybe there is a way of adding a `--CI-test` option that at least exercises the logic without needing the vcpkg to be built again (IIRC, and I may well be wrong, we build once, remember the artefacts, and then re-used them, but .. dunno). Philip
Hi Philip, On Fri, 18 Jun 2021, Philip Oakley wrote: > On 18/06/2021 14:42, Johannes Schindelin wrote: > >>> We already exercise the plain Makefile plenty, and the CMake-based build > >>> using Windows (in the `vs-build` job in `.github/workflows/main.yml`). > >> There is one 'gotcha' in the yml (probably historical) in that it > >> doesn't actually test the approach/changes that Matt addresses regarding > >> my [1]. > >> > >> That is, I'm looking at the 'out of the box' view, while the yml test > >> _preloads_ the vcpkg artefacts. > > We need to "pre-load" them because building them would add another > > whopping 20 minutes to each CI run. And I am not talking total time, but > > wall-clock time. > > > > And we're not in the business of testing vcpkg's build. > > > > So I am really not in favor of even thinking about changing this > > "pre-loading" strategy. > > > > > I can see the common sense in that, however I was trying to highlight > that the approach in patch series could go stale, as did the previous > method. Making the entry ramp to investigating the code for the wide > variety windows users should have _some_ testing.. > > I don't have any good ideas about how to get out of that 20 minute > Catch-22 issue at the moment. Maybe it needs an independent, on-demand > (i.e. infrequent;-) test. > > Maybe there is a way of adding a `--CI-test` option that at least > exercises the logic without needing the vcpkg to be built again (IIRC, > and I may well be wrong, we build once, remember the artefacts, and then > re-used them, but .. dunno). I would strongly discourage tacking this onto the current CI. It is way too rare a use case to merit adding the cost for all developers using the CI runs to verify their work. All is not lost, though: interested parties (such as yourself!) can easily add their own GitHub workflows in their own repositories and verify that things work. You could even put the workflow on a timer, and add a matrix job that builds `maint`, `master`, `next` and `seen`, to verify that things work. And for extra brownie points, you can monitor the runs and work on fixes whenever you see breakages. That would definitely take a good chunk of the maintenance burden off of the Git maintainers. Ciao, Dscho