CMake build system for git #2580
Conversation
|
Whoa, that's a pretty big change. To be honest, I am not such a big fan of CMake, but I figure that this is not exactly intrusive a change, as it merely adds a file. The trick will be to keep it working, and I bet that the best idea would be to adjust the Of course, we will still have to take care of building the - name: build
shell: bash
run: ...Also, please use your real name and real email address for the committer/author information. Looking forward to see this progress! |
|
The cmake script generates command-list.h through execute process. On windows you need to git-bash installed and have sh.exe in your path.
|
Indeed. That is precisely what I meant, when I suggested to adjust the GitHub workflow to use the new CMake workflow to verify that it keeps working. |
|
Thank you for working on this! It looks as if CMake was unable to find the Also, please do modify the |
|
I don’t know why GitHub is starting the build on git-for-windows/git. I am making changes in my fork only.
Sorry, I am new to CI/CD .
I tried triggering a workflow and ended up creating a new file, this file is just temporary.
And yes, I am not able find the artifacts. Can you tell me the directory in which the artifacts are extracted to?
And how do I stop triggering GitHub actions on git-for-windows/git?
Thank You
From: Johannes Schindelin
Sent: 11 April 2020 02:09 AM
To: git-for-windows/git
Cc: Sibi Siddharthan; Author
Subject: Re: [git-for-windows/git] CMake build system for git (#2580)
Thank you for working on this!
It looks as if CMake was unable to find the vcpkg-provided artifacts, right?
Also, please do modify the vs-build job in .github/workflows/main.yml instead of adding a new file.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
They should be extracted in-place, where they would have been put by
You can stop it from triggering by removing the If I were you, I would also remove the jobs that you're not interested in, i.e. everything except |
|
I have to admit that it is exciting to watch the progress from the sidelines... |
|
Well, the build is now succeeding in the CI system, now I have to make the tests to work.
From: Johannes Schindelin
Sent: 11 April 2020 01:11 PM
To: git-for-windows/git
Cc: Sibi Siddharthan; Author
Subject: Re: [git-for-windows/git] CMake build system for git (#2580)
I have to admit that it is exciting to watch the progress from the sidelines...
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
SibiSiddharthan
force-pushed
the
master
branch
2 times, most recently
from
e0cc99f
to
49383cb
Compare
If you worked directly in |
|
If you compare the
Related to the last point, there is also a difference in how the environment variables diff --git a/succ/bin-wrappers/git b/unsuc/bin-wrappers/git
index bbc9885..5410af4 100644
--- a/succ/bin-wrappers/git
+++ b/unsuc/bin-wrappers/git
@@ -4,20 +4,19 @@
# to run test suite against sandbox, but with only bindir-installed
# executables in PATH. The Makefile copies this into various
# files in bin-wrappers, substituting
-# /d/a/git/git and git.exe.
+# . and git.exe.
-test -n "${GIT_EXEC_PATH##*:*}" ||
- GIT_EXEC_PATH="$(cygpath -u "$GIT_EXEC_PATH")"
+GIT_EXEC_PATH='.'
if test -n "$NO_SET_GIT_TEMPLATE_DIR"
then
unset GIT_TEMPLATE_DIR
else
- GIT_TEMPLATE_DIR="$GIT_EXEC_PATH"'/templates/blt'
+ GIT_TEMPLATE_DIR='./templates/blt'
export GIT_TEMPLATE_DIR
fi
-GITPERLLIB="$GIT_EXEC_PATH"'/perl/build/lib'"${GITPERLLIB:+:$GITPERLLIB}"
-GIT_TEXTDOMAINDIR="$GIT_EXEC_PATH"'/po/build/locale'
-PATH="$GIT_EXEC_PATH"'/bin-wrappers:'"$PATH"
+GITPERLLIB='./perl/build/lib'"${GITPERLLIB:+:$GITPERLLIB}"
+GIT_TEXTDOMAINDIR='./po/build/locale'
+PATH='./bin-wrappers:'"$PATH"The reason for those modifications lives here: https://github.com/git-for-windows/git/blob/v2.26.0.windows.1/config.mak.uname#L805-L823 I agree that this is not particularly elegant, and should probably be fixed in |
|
The windows build now works. Looks like I had to add invalidcontinue.obj to the link options, which I had no idea about.
In the CI when I try to create links in the build phase, when untar(ing) tar is not able to restore the links. So I am sticking with verbatim copies for now.
The tests now run, but I get a failure on t2024 in the CI which I don’t get in my system.
The only test failures I get in my system (with NO_GETTEXT) are t0060 -> which is due to ENSURE_MSYSTEM_IS_SET and t2040 -> I think this has to do with the shell
With respect to the main.yml should I create a separate task like vs-build-cmake or should I edit vs-build itself. Please advice.
Thank You.
From: Johannes Schindelin
Sent: 11 April 2020 06:11 PM
To: git-for-windows/git
Cc: Sibi Siddharthan; Author
Subject: Re: [git-for-windows/git] CMake build system for git (#2580)
If you compare the vs-artifacts from https://github.com/SibiSiddharthan/git/runs/578782714 to the latest successful run triggered by a push to Git for Windows' master (https://github.com/git-for-windows/git/actions/runs/73638378), you will realize three things:
• The artifacts.tar.gz files are of a substantially different size: 201MB vs 14MB. The difference is most likely due to the built-ins not being hardlinked to git.exe but being verbatim copies.
• The scripts that are bundled in the failed run all have CR/LF line endings, the one from the successful run have LF-only line endings.
• The bin-wrappers/ scripts in the failed run define GIT_EXEC_PATH='.' always. The successful run's have this instead:
• test -n "${GIT_EXEC_PATH##*:*}" ||
GIT_EXEC_PATH="$(cygpath -u "$GIT_EXEC_PATH")"
Related to the last point, there is also a difference in how the environment variables GIT_TEMPLATE_DIR, GITPERLLIB, GIT_TEXTDOMAINDIRandPATHare defined. As an example, the full diff ofbin-wrappers/git`:
diff --git a/succ/bin-wrappers/git b/unsuc/bin-wrappers/git
index bbc9885..5410af4 100644
--- a/succ/bin-wrappers/git
+++ b/unsuc/bin-wrappers/git
@@ -4,20 +4,19 @@
# to run test suite against sandbox, but with only bindir-installed
# executables in PATH. The Makefile copies this into various
# files in bin-wrappers, substituting
-# /d/a/git/git and git.exe.
+# . and git.exe.
…-test -n "${GIT_EXEC_PATH##*:*}" ||
- GIT_EXEC_PATH="$(cygpath -u "$GIT_EXEC_PATH")"
+GIT_EXEC_PATH='.'
if test -n "$NO_SET_GIT_TEMPLATE_DIR"
then
unset GIT_TEMPLATE_DIR
else
- GIT_TEMPLATE_DIR="$GIT_EXEC_PATH"'/templates/blt'
+ GIT_TEMPLATE_DIR='./templates/blt'
export GIT_TEMPLATE_DIR
fi
-GITPERLLIB="$GIT_EXEC_PATH"'/perl/build/lib'"${GITPERLLIB:+:$GITPERLLIB}"
-GIT_TEXTDOMAINDIR="$GIT_EXEC_PATH"'/po/build/locale'
-PATH="$GIT_EXEC_PATH"'/bin-wrappers:'"$PATH"
+GITPERLLIB='./perl/build/lib'"${GITPERLLIB:+:$GITPERLLIB}"
+GIT_TEXTDOMAINDIR='./po/build/locale'
+PATH='./bin-wrappers:'"$PATH"
The reason for those modifications lives here: https://github.com/git-for-windows/git/blob/v2.26.0.windows.1/config.mak.uname#L805-L823
I agree that this is not particularly elegant, and should probably be fixed in wrap-for-bin.sh instead. At some stage I made a mental note about that, but had totally forgotten in the meantime.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
You're right. I had forgotten all about that.
Are those hardlinks, or symlinks? I think only the former will work (e.g.
Can you point me to verbose logs? It's hard to guess based on that description of a test failure.
I would recommend to replace the |
|
In the CI when I try to create links in the build phase, when untar(ing) tar is not able to restore the links. So I am sticking with verbatim copies for now.
I create symlinks, will try with hardlinks today.
Errors in the CI
```
T2024-checkout-dwim.sh
Failed test
t/t2024-checkout-dwim.sh:116: error: not ok 6 - checkout -p with multiple remotes does not print advice
401#
402# git checkout -B master &&
403# test_might_fail git branch -D foo &&
404#
405# git checkout -p foo 2>stderr &&
406# test_i18ngrep ! "^hint: " stderr &&
407# status_uno_is_clean
408#
T7106-reset-unborn-branch
Failed test
t/t7106-reset-unborn-branch.sh:35: error: not ok 5 - reset -p
1120#
1121# rm .git/index &&
1122# git add a &&
1123# echo y >yes &&
1124# git reset -p <yes >output &&
1125#
1126# git ls-files >actual &&
1127# test_must_be_empty actual &&
1128# test_i18ngrep "Unstage" output
1129#
```
In my PC
```
T0060-path-utils.sh
Failed test
not ok 211 - MSYSTEM/PATH is adjusted if necessary
#
# mkdir -p "$HOME"/bin pretend/mingw64/bin \
# pretend/mingw64/libexec/git-core pretend/usr/bin &&
# cp "$GIT_EXEC_PATH"/git.exe pretend/mingw64/bin/ &&
# cp "$GIT_EXEC_PATH"/git.exe pretend/mingw64/libexec/git-core/ &&
# echo "env | grep MSYSTEM=" | write_script "$HOME"/bin/git-test-home &&
# echo "echo mingw64" | write_script pretend/mingw64/bin/git-test-bin &&
# echo "echo usr" | write_script pretend/usr/bin/git-test-bin2 &&
#
# (
# MSYSTEM= &&
# GIT_EXEC_PATH= &&
# pretend/mingw64/libexec/git-core/git.exe test-home >actual &&
# pretend/mingw64/libexec/git-core/git.exe test-bin >>actual &&
# pretend/mingw64/bin/git.exe test-bin2 >>actual
# ) &&
# test_write_lines MSYSTEM=$MSYSTEM mingw64 usr >expect &&
# test_cmp expect actual
T2040-checkout-symlink-attr
Failed test
not ok 1 - checkout symlinks with attr
#
# cache_symlink file1 file-link &&
# cache_symlink dir dir-link &&
#
# printf "file-link symlink=file\ndir-link symlink=dir\n" >.gitattributes &&
# git add .gitattributes &&
#
# git checkout . &&
#
# mkdir dir &&
# echo "[a]b=c" >file1 &&
# echo "[x]y=z" >dir/file2 &&
#
# # MSYS2 is very forgiving, it will resolve symlinks even if the
# # symlink type is incorrect. To make this test meaningful, try
# # them with a native, non-MSYS executable, such as `git config`.
# test "$(git config -f file-link a.b)" = "c" &&
# test "$(git config -f dir-link/file2 x.y)" = "z"
#
```
I also tested in my PC with the created artifacts of my build in the CI and get the same error as in the CI.
Compiler version issue?
|
|
Would it be possible to include the trace of the failed tests (i.e. the part preceding the code you showed)? |
|
The builds works now. Yippee!!!
T0060-path-utils-sh was failing because it was not able to load libiconv.dll
T2040-checkout-symlink-attr.h is a MINGW only test, so no need to bother with it on MSVC, am I right?
The other tests that were failing in the CI were because of (NO_PERL=1) . The tests worked fine on my system with perl, but fail in the CI. Can you explain why?. Is it because the tar doesn't include the .pm files?
For the merge, I have two dummy commits which are unsigned. Should I rebase with the original PR(commit) and add these changes, or is it fine?
Clang also works now, so should I add a Clang build to main.yml?
From: Johannes Schindelin
Sent: 12 April 2020 03:52 PM
To: git-for-windows/git
Cc: Sibi Siddharthan; Author
Subject: Re: [git-for-windows/git] CMake build system for git (#2580)
Would it be possible to include the trace of the failed tests (i.e. the part preceding the code you showed)
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
Cool!
And that
Actually, in Git's test suite, the So I think we still need that test to pass.
Oh, and the tests were not run with
It probably is. And we don't really want to include them.
Are those merge commits necessary? Otherwise I'd just The PR build should test a merge, anyway.
Personally, I wouldn't. If we really want to test that, I would modify The benefit for MSVC, OTOH, is obvious, so changing |
|
T0060-path-utils-sh was failing in my local tests not the CI one but I fixed it mine as well
T2040-checkout-symlink-attr.h still fails locally, but in the CI it passes, have no idea why.
The reason I prefer clang is because it is a better compiler than MSVC. Hope to see it added soon.
Will do the changes shortly.
This is the log output from T2040
COMMAND cd t/ && sh T2040-checkout-symlink-attr.h -v -x
Initialized empty Git repository in D:/my/git-win/t/trash directory.t2040-checkout-symlink-attr/.git/
checking prerequisite: SYMLINKS
mkdir -p "$TRASH_DIRECTORY/prereq-test-dir" &&
(
cd "$TRASH_DIRECTORY/prereq-test-dir" &&
# test whether the filesystem supports symbolic links
ln -s x y && test -h y
)
++ mkdir -p '/d/my/git-win/t/trash directory.t2040-checkout-symlink-attr/prereq-test-dir'
++ cd '/d/my/git-win/t/trash directory.t2040-checkout-symlink-attr/prereq-test-dir'
++ ln -s x y
++ test -h y
prerequisite SYMLINKS ok
expecting success of 2040.1 'checkout symlinks with attr':
cache_symlink file1 file-link &&
cache_symlink dir dir-link &&
printf "file-link symlink=file\ndir-link symlink=dir\n" >.gitattributes &&
git add .gitattributes &&
git checkout . &&
mkdir dir &&
echo "[a]b=c" >file1 &&
echo "[x]y=z" >dir/file2 &&
# MSYS2 is very forgiving, it will resolve symlinks even if the
# symlink type is incorrect. To make this test meaningful, try
# them with a native, non-MSYS executable, such as `git config`.
test "$(git config -f file-link a.b)" = "c" &&
test "$(git config -f dir-link/file2 x.y)" = "z"
++ cache_symlink file1 file-link
+++ printf %s file1
+++ git hash-object --stdin -w
++ sha=08219db9b0969fa29cf16fd04df4a63964da0b69
++ git update-index --add --cacheinfo 120000,08219db9b0969fa29cf16fd04df4a63964da0b69,file-link
++ cache_symlink dir dir-link
+++ printf %s dir
+++ git hash-object --stdin -w
++ sha=87245193225f8ff56488ceab0dcd11467fe098d0
++ git update-index --add --cacheinfo 120000,87245193225f8ff56488ceab0dcd11467fe098d0,dir-link
++ printf 'file-link symlink=file\ndir-link symlink=dir\n'
++ git add .gitattributes
++ git checkout .
Updated 2 paths from the index
++ mkdir dir
++ echo '[a]b=c'
++ echo '[x]y=z'
+++ git config -f file-link a.b
++ test '' = c
error: last command exited with $?=1
not ok 1 - checkout symlinks with attr
#
# cache_symlink file1 file-link &&
# cache_symlink dir dir-link &&
#
# printf "file-link symlink=file\ndir-link symlink=dir\n" >.gitattributes &&
# git add .gitattributes &&
#
# git checkout . &&
#
# mkdir dir &&
# echo "[a]b=c" >file1 &&
# echo "[x]y=z" >dir/file2 &&
#
# # MSYS2 is very forgiving, it will resolve symlinks even if the
# # symlink type is incorrect. To make this test meaningful, try
# # them with a native, non-MSYS executable, such as `git config`.
# test "$(git config -f file-link a.b)" = "c" &&
# test "$(git config -f dir-link/file2 x.y)" = "z"
#
# failed 1 among 1 test(s)
1..1
|
|
The cmake build works. All tests pass. The generated artifacts use hardlinks now. |
|
Excellent! About the t2040 failure: do you use native symlinks in MSYS2 by default? IIRC you have to set the environment variable
You mean Git for Windows? I dabbled with the idea, but Besides, there was rumour in the MSYS2 project that they might want to switch from GCC to clang in their own MINGW packages, and I was kinda thinking that this switch would make for a perfect excuse to do the same for |
|
I don’t use MSYS2, I use Git-Bash(I know it ships with a msys2 runtime) that is shipped with the git for windows installer.
I didn’t mean mingw-clang, I meant clang with the MSVC runtime and headers.
In my experience for STD C functions the MSVC ones are better than the mingw ones.
In the CI I checked the logs vs-test(5) t2040 is run and is not skipped
Any more changes I have to make to the commits?
From: Johannes Schindelin
Sent: 13 April 2020 12:06 PM
To: git-for-windows/git
Cc: Sibi Siddharthan; Author
Subject: Re: [git-for-windows/git] CMake build system for git (#2580)
Excellent!
About the t2040 failure: do you use native symlinks in MSYS2 by default? IIRC you have to set the environment variable MSYS to something containing the substring "nativelinks". That would explain why this test case is run at all. I don't think that our CI runs with that flag.
The reason I prefer clang is because it is a better compiler than MSVC. Hope to see it added soon.
You mean Git for Windows? I dabbled with the idea, but mingw-w64-clang is not installed in Git for Windows SDK by default, and it would add a noticeable amount of megabytes to the already large SDK.
Besides, there was rumour in the MSYS2 project that they might want to switch from GCC to clang in their own MINGW packages, and I was kinda thinking that this switch would make for a perfect excuse to do the same for mingw-w64-git.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
There was a problem hiding this comment.
Wow, what a gigantic amount of work! Well done.
I offered a huge amount of comments, suggestions, questions in response, and in addition I would like to propose a commit structure to make this easier to review on the Git mailing list (where I want to send this eventually):
- preparatory patches (such as modifying
t/test-lib.shto allow for the build directory being separate from the source directory) - introduce a basic
CMakeLists.txt. Maybe just the bare-bones one that can build the executables, without gettext, only on Linux. - a bunch of commits to enhance
CMakeLists.txtin easy-to-describe steps, e.g.add support for gettext,also "build" the templates,support building with macOS,support building with MSVC,support building with clang on Windows, etc - a final patch that modifies
.github/workflows/main.ymlso that it exercises the CMake-based build invs-build, mentioning in the commit message why we do it, why only here, that having this in the CI/PR builds will make it easier to keep this working, and whatever else we think might be useful to keep in mind for the keen reader.
How does that sound?
| # | ||
| # Copyright (c) 2020 Sibi Siddharthan | ||
| # | ||
| cmake_minimum_required(VERSION 3.15) |
There was a problem hiding this comment.
According to https://blog.kitware.com/cmake-3-15-0-available-for-download/, that version is not even a year old. Maybe we can lower the expectations? Or are there features required by this file that are available only in CMake v3.15.0 and newer?
There was a problem hiding this comment.
According to the cmake docs, the lowest I can go is 3.12.4.(Need to test) This was the first release with add_compile_options() which is used throughout build script. This means I don't have a common way of creating create symlinks anymore. I think this is okay, because in CI creating hardlinks is better.
There was a problem hiding this comment.
As the " Added support for MSVC and clang on Windows" commit (whose first line should probably read more like "cmake: support MSVC and clang on Windows") seems to require CMake v3.15, could you describe in that commit's message why?
There was a problem hiding this comment.
Oh, since I think of this right now: the biggest difference between the support for GCC (MINGW) on Windows and the support for MSVC/clang on Windows is that the former is expected to be built and run in Git for Windows' SDK, and the latter can easily run without the full SDK, apart from the compiler it just needs a Git Bash from an end-user Git for Windows installation (which is much more light-weight than the full SDK).
I think this is really a distinction worth mentioning in both of the commits, the one introducing support for GCC (MINGW) and the one introducing support for MSVC/clang.
Also, I think that we may want to auto-initialize the vcpkg system similar to https://github.com/git-for-windows/git/blob/v2.26.1.windows.1/config.mak.uname#L29 (i.e. we should run compat/vcbuild/vcpkg_install.bat unless compat/vcbuild/vcpkg/installed/ exists).
|
Thanks for your review,
Does this roadmap sound good to you? |
|
When we create symlinks and tar the build artifacts and untar them for testing tar is not able to resolve the symlinks. So that is why hardlinks were chosen.
|
|
That roadmap sounds good. I'd suggest to follow https://git-scm.com/docs/git-rebase#_splitting_commits to split it up, then force-push. No need to open a new PR. About the hardlinks and |
|
I have also tested the CMake script with the git/git repo. There are only 3 changes that I have to make for it to work there(These changes are undoing the compatibility stuff for this fork and git/git). So if I make these changes the script would not work here. |
Yes, I think that would be an excellent idea! You will probably want to use either |
|
git/next and dd/ci-swap-azure-pipelines-with-github-actions are same(as of today). Just to clarify any PRs I raise to gitgitgadget/git to any branch will be in the mailing lists (git@vger.kernel.org), right? And, I tested without the CI specific blocks, since each job takes place in the same directory(wrt path) the build works.
|
Not quite. The former includes the latter, but the former includes much more in addition.
Yes, you will need to
Yes, it's good that the CI-specific blocks are gone. It would still be good to support (somehow, e.g. using the "t/test-lib.sh: set |
|
@dscho Regarding this PR, do you want a CMake script specific to this repo? (depending on Makefiles on sources ofcourse). |
|
@SibiSiddharthan oh, just go ahead with contrib/ and work with the Git mailing list to get it integrated into |
|
Oh, there is one more thing that I would like to address, but I struggle to find the time: the portable Git should be as fast as git-sdk-64-minimal, if not faster. My guess is that if we replace then it should be as fast. |
|
It is not about git-sdk-64-minimal or git-64-portable that causes the slowdown. Calling git-cmd then calling bash is the problem. |
I don't actually think that But there is this a bigger difference between git-sdk-64-minimal and portable Git: the That's why I think that overwriting git-64-portable's It is also quite possible, though, that the |
|
@dscho The CMake patch series has been merged into ss/cmake-build. Thank you for your support!!! |
|
Yeah! |
|
@dscho quite a few of the tests are failing for vs-build in the CI. |
|
@dscho I think I have found the issue causing some tests to fail, it is related to multibyte characters. When I do a local compile with the same version of CMake , compiler I get no errors. |
Can you give me a link to the log? |
This is one of the failed runs. |
I see this in the log: Which suggests to me that this might be compiled without It would seem to me that it looks at the wrong |
Yep, this is the issue. Don't understand why it is selecting that libiconv. The library search paths start with CMAKE_PREFIX_PATH, which I specify. Looks like I need to override libiconv location manually. Now I am worried, in the future if the github environment installs another package which overrides one of the required libraries, a similar problem can arise. I think it is better to specify the location of the libraries manually for avoiding future issues like these. |
|
If you use CMake 3.17 and up, |
Is there maybe a way to dump the actual search path that's used? I am not so sure that the paths we want to use are prefixed, they might be appended instead.
Ah, that's exactly the answer ;-)
I would like to avoid that, honestly. We might need to find a way to hard-code the search path, though, so that none but the pre-built libraries are picked up. |
|
Figured out why cmake was finding postgresql's iconv.lib, instead of vcpkg's one. This is because cmake prefers iconv.lib to libiconv.lib in their implementation. Even though it found vcpkg's libiconv.lib first it preferred the other one. |
|
It's hard to tell in general which name is preferred overall because that is a deployment decision, not something CMake can just assume. Given that there's no real way to express this preference today, I'd suggest (in decreasing order of preference):
|
This is what I am using as a workaround in the CI.
Is there a way ,say a variable we can set so that all calls to find_* commands use NAMES_PER_DIR? |
No, each package is different in this way. It should probably be used for any |
|
Thank you, @SibiSiddharthan, for your incredible patience and energy that you put into this project. Through your hard work, this feature entered git/git first and is now part of v2.29.0-rc0.windows.1. 🎉 |
|
@dscho thanks a lot for your support too. |
This is cmake build system for git.
Tested platforms
MinGW GCC 9.2
Clang 9
Visual Studio 2015,2019