diff mbox series

[v2,11/11] ci: modification of main.yml to use cmake for vs-build job

Message ID fa1b8032906c6042a0e5851f803ec0427922a1a5.1589302255.git.gitgitgadget@gmail.com (mailing list archive)
State New, archived
Headers show
Series CMake build system for git | expand

Commit Message

Linus Arver via GitGitGadget May 12, 2020, 4:50 p.m. UTC 
From: Sibi Siddharthan <sibisiddharthan.github@gmail.com>

This patch modifies .github/workflows/main.yml to use CMake for
Visual Studio builds.

Modified the vs-test step to match windows-test step. This speeds
up the vs-test. Calling git-cmd from powershell and then calling git-bash
to perform the tests slows things down(factor of about 6). So git-bash
is directly called from powershell to perform the tests using prove.

NOTE: Since GitHub keeps the same directory for each job
(with respect to path) absolute paths are used in the bin-wrapper
scripts.

GitHub has switched to CMake 3.17.1 which changed the behaviour of
FindCURL module. An extra definition (-DCURL_NO_CURL_CMAKE=ON) has been
added to revert to the old behaviour.

Edit(Explanation for the reordering of build steps):
In the configuration phase CMake looks for the required libraries for
building git (eg zlib,libiconv). So we extract the libraries before we
configure.

Changes:
The CMake script has been relocated to contib/buildsystems, so point
to the CMakeLists.txt in the invocation commands.

The generation command now uses the absolute paths for the generation
step.

To check for ICONV_OMITS_BOM libiconv.dll needs to be in the working
directory of script or path. So we copy the dlls before we configure.

Signed-off-by: Sibi Siddharthan <sibisiddharthan.github@gmail.com>
---
 .github/workflows/main.yml | 46 +++++++++++++++++++++-----------------
 1 file changed, 26 insertions(+), 20 deletions(-)

Comments

Junio C Hamano May 12, 2020, 9:27 p.m. UTC | #1 
"Sibi Siddharthan via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Sibi Siddharthan <sibisiddharthan.github@gmail.com>
> Subject: Re: [PATCH v2 11/11] ci: modification of main.yml to use cmake for vs-build job

Yay!

> This patch modifies .github/workflows/main.yml to use CMake for
> Visual Studio builds.
> Modified the vs-test step to match windows-test step. This speeds

	Teach .github/workflows/main.yml to use Cmake for VS builds.
	Modify vs-test step to match windows-test step to speed up
	the vs-test.

> Changes:
> The CMake script has been relocated to contib/buildsystems, so point
> to the CMakeLists.txt in the invocation commands.

This does not belong to the log message for this (or any for that
matter) commit, no?  Up to this point, nothing in main.yml used
CMake script from anywhere, and "has been relocated" is irrelevant.

And if we add CMake script to contrib/buildsystems/ from the
beginning in [01/11], there won't be any "relocation" the readers of
the log message of this step need to know about.

> The generation command now uses the absolute paths for the generation
> step.

"now uses"?  Is that something the readers of the log message of
this step need to know about, or is it "in contrast to an earlier
attempt to add CMake script"?  If the latter, it only confuses the
readers of our history, as most of them will not even know about an
earlier attempt.

Thanks.
Sibi Siddharthan May 13, 2020, 7:45 p.m. UTC | #2 
On Wed, May 13, 2020 at 2:57 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Sibi Siddharthan via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > From: Sibi Siddharthan <sibisiddharthan.github@gmail.com>
> > Subject: Re: [PATCH v2 11/11] ci: modification of main.yml to use cmake for vs-build job
>
> Yay!
>
> > This patch modifies .github/workflows/main.yml to use CMake for
> > Visual Studio builds.
> > Modified the vs-test step to match windows-test step. This speeds
>
>         Teach .github/workflows/main.yml to use Cmake for VS builds.
>         Modify vs-test step to match windows-test step to speed up
>         the vs-test.
>
> > Changes:
> > The CMake script has been relocated to contib/buildsystems, so point
> > to the CMakeLists.txt in the invocation commands.
>
> This does not belong to the log message for this (or any for that
> matter) commit, no?  Up to this point, nothing in main.yml used
> CMake script from anywhere, and "has been relocated" is irrelevant.
>

Will remove this.

> And if we add CMake script to contrib/buildsystems/ from the
> beginning in [01/11], there won't be any "relocation" the readers of
> the log message of this step need to know about.
>
> > The generation command now uses the absolute paths for the generation
> > step.
>
> "now uses"?  Is that something the readers of the log message of
> this step need to know about, or is it "in contrast to an earlier
> attempt to add CMake script"?  If the latter, it only confuses the
> readers of our history, as most of them will not even know about an
> earlier attempt.

Will also remove this.

>
> Thanks.

Thank You,
Sibi Siddharthan
Sibi Siddharthan May 25, 2020, 7:16 p.m. UTC | #3 
Hi Junio,

I have finished the changes you have asked for.
1) Relocating the CMake script to contrib/buildsystems from patch 01/xx.
2) Parse the Makefile for sources from patch 01/xx.
3) Reworded the commit messages you pointed out.
4) Rebased the ST_BLOCKS_IN_STRUCT_STAT and ICONV_OMITS_BOM to patch
01/xx to make the review process easier.

No new features will be introduced in the script, to make the review
process easier.

I have looked at the GIT-VERSION-GEN script and the logic it uses to
determine the version of git.
The logic is a bit complicated to be implemented in a  CMake script,
so I am skipping it for now.

Any other changes I should make before I submit PATCH v3?

Thank You,
Sibi Siddharthan
Junio C Hamano May 25, 2020, 8:03 p.m. UTC | #4 
Sibi Siddharthan <sibisiddharthan.github@gmail.com> writes:

> 1) Relocating the CMake script to contrib/buildsystems from patch 01/xx.
> 2) Parse the Makefile for sources from patch 01/xx.
> 3) Reworded the commit messages you pointed out.
> 4) Rebased the ST_BLOCKS_IN_STRUCT_STAT and ICONV_OMITS_BOM to patch
> 01/xx to make the review process easier.
> ...
> No new features will be introduced in the script, to make the review
> process easier.

You did not answer <xmqqd077qnqc.fsf@gitster.c.googlers.com>,
so I have to guess.

I am guessing that your answer would be that if we keep this series
concentrate only on Windows support from the beginning and do not
add support for any other platform at all, the scope and necessary
effort to bring the patches in reviewable shape would be reduced
dramatically.  And if that is the case, that would be great.

In other words, I'd prefer not just "no new features", but "with
much less features, now we do not even add support for Linux or
macOS to the initial patch series".

But because I haven't heard the question answered, I cannot tell if
that level of simplification is possible and if that is already what
you did in (silent) response to my questions.

> I have looked at the GIT-VERSION-GEN script and the logic it uses to
> determine the version of git.
> The logic is a bit complicated to be implemented in a  CMake script,
> so I am skipping it for now.

Regarding the generation of GIT-VERSION-FILE, I asked you why you
need to hand-parse, instead of running the script itself, which is
the way you generate the config-list.h file in the same patch, which
is done with execute_process() to run generate-cmdlist.sh.

You did not answer the question, either, so I have to guess.

I am guessing that it was an oversight that you did not update the
procedure to handle GIT-VERSION-FILE because you wrote the "let's
string-find in the GIT-VERSION-GEN script" approach way before you
wrote the other parts of the CMake thing, and with the experience
and hindsight of having written command-list-h support, you now
realize that running the script to ensure that you get the result
consistent with folks who use Makefile would be much better.  After
all, the logic being more complex than your CMake support can
express shouldn't be an issue, if you just let the logic execute
itself, no?

But that is also merely my guess, so...

> Any other changes I should make before I submit PATCH v3?

... at this point, it is too early for me to answer that question.
Sibi Siddharthan May 25, 2020, 8:56 p.m. UTC | #5 
On Tue, May 26, 2020 at 1:33 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Sibi Siddharthan <sibisiddharthan.github@gmail.com> writes:
>
> > 1) Relocating the CMake script to contrib/buildsystems from patch 01/xx.
> > 2) Parse the Makefile for sources from patch 01/xx.
> > 3) Reworded the commit messages you pointed out.
> > 4) Rebased the ST_BLOCKS_IN_STRUCT_STAT and ICONV_OMITS_BOM to patch
> > 01/xx to make the review process easier.
> > ...
> > No new features will be introduced in the script, to make the review
> > process easier.
>
> You did not answer <xmqqd077qnqc.fsf@gitster.c.googlers.com>,
> so I have to guess.

Sorry, was going to reply to that, but forgot about it along the way.

As for my reply to that for windows only support we do not need
a few of the checks namely:
1) HAVE_BSD_SYSCTL
2) ICONV_OMITS_BOM (because we are using GNU libiconv 1.16)

The functions checks (check_function_exists) is needed.
A few of the header checks are unnecessary(paths.h sys/sysinfo.h).

The difference in build between Linux platforms and Windows would
be 8-15 lines of code atmost . I see no point in the extra Linux stuff to be
a burden for the reviewers.

There is another way of just copying the configurations in config.mak.uname,
but this hurts the purpose of configuring in the first place.

>
> I am guessing that your answer would be that if we keep this series
> concentrate only on Windows support from the beginning and do not
> add support for any other platform at all, the scope and necessary
> effort to bring the patches in reviewable shape would be reduced
> dramatically.  And if that is the case, that would be great.
>
> In other words, I'd prefer not just "no new features", but "with
> much less features, now we do not even add support for Linux or
> macOS to the initial patch series".

macOS support is ways off, needs quite a bit of work.

In terms of Linux support apart from the function, header checks there
is nothing special
implemented here.

>
> But because I haven't heard the question answered, I cannot tell if
> that level of simplification is possible and if that is already what
> you did in (silent) response to my questions.
>
> > I have looked at the GIT-VERSION-GEN script and the logic it uses to
> > determine the version of git.
> > The logic is a bit complicated to be implemented in a  CMake script,
> > so I am skipping it for now.
>
> Regarding the generation of GIT-VERSION-FILE, I asked you why you
> need to hand-parse, instead of running the script itself, which is
> the way you generate the config-list.h file in the same patch, which
> is done with execute_process() to run generate-cmdlist.sh.
>
> You did not answer the question, either, so I have to guess.
>
> I am guessing that it was an oversight that you did not update the
> procedure to handle GIT-VERSION-FILE because you wrote the "let's
> string-find in the GIT-VERSION-GEN script" approach way before you
> wrote the other parts of the CMake thing, and with the experience
> and hindsight of having written command-list-h support, you now
> realize that running the script to ensure that you get the result
> consistent with folks who use Makefile would be much better.  After
> all, the logic being more complex than your CMake support can
> express shouldn't be an issue, if you just let the logic execute
> itself, no?
>

Yes, fair point.

Have executed GIT-VERSION-GEN, then parsed the GIT-VERSION-FILE for
version info.
Added it to the list of changes.

> But that is also merely my guess, so...
>
> > Any other changes I should make before I submit PATCH v3?
>
> ... at this point, it is too early for me to answer that question.

Thank You,
Sibi Siddharthan
Johannes Schindelin May 25, 2020, 9:40 p.m. UTC | #6 
Hi Sibi,

On Tue, 26 May 2020, Sibi Siddharthan wrote:

> The difference in build between Linux platforms and Windows would be
> 8-15 lines of code atmost . I see no point in the extra Linux stuff to
> be a burden for the reviewers.

I agree that this does not constitute a big burden.

Linux support in the CMake system _might_ come in real handy in certain
circumstances. For example, if I understand correctly cppcheck uses the
CMake configuration to figure out which source files to analyze, and with
which #define's.

So there might be some previously-underappreciated benefit to having a
CMake configuration that works on Linux, even if it is not the recommended
way to build Git on Linux.

Thank you for your incredible work on the CMake front,
Dscho
diff mbox series

Patch

diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml
index fd4df939b50..7a65cc0764f 100644
--- a/.github/workflows/main.yml
+++ b/.github/workflows/main.yml
@@ -80,13 +80,6 @@  jobs:
     - name: download git-sdk-64-minimal
       shell: bash
       run: a=git-sdk-64-minimal && mkdir -p $a && curl -# https://wingit.blob.core.windows.net/ci-artifacts/$a.tar.xz | tar -C $a -xJf -
-    - name: generate Visual Studio solution
-      shell: powershell
-      run: |
-        & .\git-sdk-64-minimal\usr\bin\bash.exe -lc @"
-          make NDEBUG=1 DEVELOPER=1 vcxproj
-        "@
-        if (!$?) { exit(1) }
     - name: download vcpkg artifacts
       shell: powershell
       run: |
@@ -98,6 +91,14 @@  jobs:
         Remove-Item compat.zip
     - name: add msbuild to PATH
       uses: microsoft/setup-msbuild@v1.0.0
+    - name: copy dlls to root
+      shell: powershell
+      run: |
+        & compat\vcbuild\vcpkg_copy_dlls.bat release
+        if (!$?) { exit(1) }
+    - name: generate Visual Studio solution
+      shell: bash
+      run: cmake `pwd`/contrib/buildsystems/ -DCMAKE_PREFIX_PATH=`pwd`/compat/vcbuild/vcpkg/installed/x64-windows -DMSGFMT_EXE=`pwd`/git-sdk-64-minimal/mingw64/bin/msgfmt.exe -DPERL_TESTS=OFF -DPYTHON_TESTS=OFF -DCURL_NO_CURL_CMAKE=ON
     - name: MSBuild
       run: msbuild git.sln -property:Configuration=Release -property:Platform=x64 -maxCpuCount:4 -property:PlatformToolset=v142
     - name: bundle artifact tar
@@ -106,8 +107,6 @@  jobs:
         MSVC: 1
         VCPKG_ROOT: ${{github.workspace}}\compat\vcbuild\vcpkg
       run: |
-        & compat\vcbuild\vcpkg_copy_dlls.bat release
-        if (!$?) { exit(1) }
         & git-sdk-64-minimal\usr\bin\bash.exe -lc @"
           mkdir -p artifacts &&
           eval \"`$(make -n artifacts-tar INCLUDE_DLLS_IN_ARTIFACTS=YesPlease ARTIFACTS_DIRECTORY=artifacts 2>&1 | grep ^tar)\"
@@ -125,9 +124,9 @@  jobs:
         nr: [0, 1, 2, 3, 4, 5, 6, 7, 8, 9]
     steps:
     - uses: actions/checkout@v1
-    - name: download git-64-portable
+    - name: download git-sdk-64-minimal
       shell: bash
-      run: a=git-64-portable && mkdir -p $a && curl -# https://wingit.blob.core.windows.net/ci-artifacts/$a.tar.xz | tar -C $a -xJf -
+      run: a=git-sdk-64-minimal && mkdir -p $a && curl -# https://wingit.blob.core.windows.net/ci-artifacts/$a.tar.xz | tar -C $a -xJf -
     - name: download build artifacts
       uses: actions/download-artifact@v1
       with:
@@ -136,23 +135,30 @@  jobs:
     - name: extract build artifacts
       shell: bash
       run: tar xf artifacts.tar.gz
-    - name: test (parallel)
+    - name: test
       shell: powershell
       env:
         MSYSTEM: MINGW64
         NO_SVN_TESTS: 1
         GIT_TEST_SKIP_REBASE_P: 1
       run: |
-        & git-64-portable\git-cmd.exe --command=usr\bin\bash.exe -lc @"
-          # Let Git ignore the SDK and the test-cache
-          printf '%s\n' /git-64-portable/ /test-cache/ >>.git/info/exclude
+        & .\git-sdk-64-minimal\usr\bin\bash.exe -lc @"
+          # Let Git ignore the SDK
+          printf '%s\n' /git-sdk-64-minimal/ >>.git/info/exclude
 
-          cd t &&
-          PATH=\"`$PWD/helper:`$PATH\" &&
-          test-tool.exe run-command testsuite --jobs=10 -V -x --write-junit-xml \
-                  `$(test-tool.exe path-utils slice-tests \
-                          ${{matrix.nr}} 10 t[0-9]*.sh)
+          ci/run-test-slice.sh ${{matrix.nr}} 10
         "@
+    - name: ci/print-test-failures.sh
+      if: failure()
+      shell: powershell
+      run: |
+        & .\git-sdk-64-minimal\usr\bin\bash.exe -lc ci/print-test-failures.sh
+    - name: Upload failed tests' directories
+      if: failure() && env.FAILED_TEST_ARTIFACTS != ''
+      uses: actions/upload-artifact@v1
+      with:
+        name: failed-tests-windows
+        path: ${{env.FAILED_TEST_ARTIFACTS}}
   regular:
     strategy:
       matrix: