-
Notifications
You must be signed in to change notification settings - Fork 105
Prepare for v2.29.0 #291
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Prepare for v2.29.0 #291
Conversation
At the moment, the recommended way to configure Git's builds is to simply run `make`. If that does not work, the recommended strategy is to look at the top of the `Makefile` to see whether any "Makefile knob" has to be turned on/off, e.g. `make NO_OPENSSL=YesPlease`. Alternatively, Git also has an `autoconf` setup which allows configuring builds via `./configure [<option>...]`. Both of these options are fine if the developer works on Unix or Linux. But on Windows, we have to jump through hoops to configure a build (read: we force the user to install a full Git for Windows SDK, which occupies around two gigabytes (!) on disk and downloads about three quarters of a gigabyte worth of Git objects). The build infrastructure for Git is written around being able to run make, which is not supported natively on Windows. To help Windows developers a CMake build script is introduced here. With a working support CMake, developers on Windows need only install CMake, configure their build, load the generated Visual Studio solution and immediately start modifying the code and build their own version of Git. Likewise, developers on other platforms can use the convenient GUI tools provided by CMake to configure their build. So let's start building CMake support for Git. This is only the first step, and to make it easier to review, it only allows for configuring builds on the platform that is easiest to configure for: Linux. The CMake script checks whether the headers are present(eg. libgen.h), whether the functions are present(eg. memmem), whether the funtions work properly (eg. snprintf) and generate the required compile definitions for the platform. The script also searches for the required libraries, if it fails to find the required libraries the respective executables won't be built.(eg. If libcurl is not found then git-remote-http won't be built). This will help building Git easier. With a CMake script an out of source build of git is possible resulting in a clean source tree. Note: this patch asks for the minimum version v3.14 of CMake (which is not all that old as of time of writing) because that is the first version to offer a platform-independent way to generate hardlinks as part of the build. This is needed to generate all those hardlinks for the built-in commands of Git. Signed-off-by: Sibi Siddharthan <[email protected]> Signed-off-by: Junio C Hamano <[email protected]>
…ions Implement the placeholder substitution to generate scripted Porcelain commands, e.g. git-request-pull out of git-request-pull.sh Generate shell/perl/python scripts and template using CMake instead of using sed like the build procedure in the Makefile does. The text translations are only build if `msgfmt` is found in your path. NOTE: The scripts and templates are generated during configuration. Signed-off-by: Sibi Siddharthan <[email protected]> Signed-off-by: Junio C Hamano <[email protected]>
Install the built binaries and scripts using CMake This is very similar to `make install`. By default the destination directory(DESTDIR) is /usr/local/ on Linux To set a custom installation path do this: cmake `relative-path-to-srcdir` -DCMAKE_INSTALL_PREFIX=`preferred-install-path` Then run `make install` Signed-off-by: Sibi Siddharthan <[email protected]> Signed-off-by: Junio C Hamano <[email protected]>
This patch provides an alternate way to test git using ctest. CTest ships with CMake, so there is no additional dependency being introduced. To perform the tests with ctest do this after building: ctest -j[number of jobs] NOTE: -j is optional, the default number of jobs is 1 Each of the jobs does this: cd t/ && sh t[something].sh The reason for using CTest is that it logs the output of the tests in a neat way, which can be helpful during diagnosis of failures. After the tests have run ctest generates three log files located in `build-directory`/Testing/Temporary/ These log files are: CTestCostData.txt: This file contains the time taken to complete each test. LastTestsFailed.log: This log file contains the names of the tests that have failed in the run. LastTest.log: This log file contains the log of all the tests that have run. A snippet of the file is given below. 10/901 Testing: D:/my/git-master/t/t0009-prio-queue.sh 10/901 Test: D:/my/git-master/t/t0009-prio-queue.sh Command: "sh.exe" "D:/my/git-master/t/t0009-prio-queue.sh" Directory: D:/my/git-master/t "D:/my/git-master/t/t0009-prio-queue.sh" Output: ---------------------------------------------------------- ok 1 - basic ordering ok 2 - mixed put and get ok 3 - notice empty queue ok 4 - stack order passed all 4 test(s) 1..4 <end of output> Test time = 1.11 sec NOTE: Testing only works when building in source for now. Signed-off-by: Sibi Siddharthan <[email protected]> Signed-off-by: Junio C Hamano <[email protected]>
This patch allows git to be tested when performin out of source builds. This involves changing GIT_BUILD_DIR in t/test-lib.sh to point to the build directory. Also some miscellaneous copies from the source directory to the build directory. The copies are: t/chainlint.sed needed by a bunch of test scripts po/is.po needed by t0204-gettext-rencode-sanity mergetools/tkdiff needed by t7800-difftool contrib/completion/git-prompt.sh needed by t9903-bash-prompt contrib/completion/git-completion.bash needed by t9902-completion contrib/svn-fe/svnrdump_sim.py needed by t9020-remote-svn NOTE: t/test-lib.sh is only modified when tests are run not during the build or configure. The trash directory is still srcdir/t Signed-off-by: Sibi Siddharthan <[email protected]> Signed-off-by: Junio C Hamano <[email protected]>
This patch facilitates building git on Windows with CMake using MinGW NOTE: The funtions unsetenv and hstrerror are not checked in Windows builds. Reasons NO_UNSETENV is not compatible with Windows builds. lines 262-264 compat/mingw.h compat/mingw.h(line 25) provides a definition of hstrerror which conflicts with the definition provided in git-compat-util.h(lines 733-736). To use CMake on Windows with MinGW do this: cmake `relative-path-to-srcdir` -G "MinGW Makefiles" Signed-off-by: Sibi Siddharthan <[email protected]> Signed-off-by: Junio C Hamano <[email protected]>
This patch adds support for Visual Studio and Clang builds The minimum required version of CMake is upgraded to 3.15 because this version offers proper support for Clang builds on Windows. Libintl is not searched for when building with Visual Studio or Clang because there is no binary compatible version available yet. NOTE: In the link options invalidcontinue.obj has to be included. The reason for this is because by default, Windows calls abort()'s instead of setting errno=EINVAL when invalid arguments are passed to standard functions. This commit explains it in detail: 4b623d8 On Windows the default generator is Visual Studio,so for Visual Studio builds do this: cmake `relative-path-to-srcdir` NOTE: Visual Studio generator is a multi config generator, which means that Debug and Release builds can be done on the same build directory. For Clang builds do this: On bash CC=clang cmake `relative-path-to-srcdir` -G Ninja -DCMAKE_BUILD_TYPE=[Debug or Release] On cmd set CC=Clang cmake `relative-path-to-srcdir` -G Ninja -DCMAKE_BUILD_TYPE=[Debug or Release] Signed-off-by: Sibi Siddharthan <[email protected]> Signed-off-by: Junio C Hamano <[email protected]>
Teach .github/workflows/main.yml to use CMake for VS 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. In the configuration phase CMake looks for the required libraries for building git (eg zlib,libiconv). So we extract the libraries before we configure. 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 <[email protected]> Signed-off-by: Junio C Hamano <[email protected]>
derrickstolee
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this careful layout of commits! This will definitely help the rebase.
I kicked off a new set of actions builds because of some p4-related errors on one macOS build. Not your issue.
jeffhostetler
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Thanks!!!
|
|
||
| if ( $command eq "get" ) { | ||
| my ($sha1) = packet_txt_read() =~ /^sha1=([0-9a-f]{40})$/; | ||
| my ($sha1) = packet_txt_read() =~ /^sha1=([0-9a-f]{40,64})$/; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's hard to tell what's going on in this snippet, so I'll ask a maybe dumb question.
The right hand side has /^sha1= in the pattern. Does that pattern need to change too?
That is, would whatever is generating this token stream send a line with sha1 and a 64 byte hash
or would it be either /sha1=..40 || /sha256=..64 or something like that ??
Again, not sure the context here, so just asking.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. This is actually a hard-coded part of the read-object "protocol":
Line 1030 in 22fe988
| err = packet_write_fmt_gently(process->in, "sha1=%s\n", oid_to_hex(oid)); |
The question now is whether we really want to be precise and change that part of the protocol. That would mean that the VFSforGit code would have to change accordingly... I'm not sure that I want to open that can of worms.
Yes, sure, technically it would be incorrect to ask for a SHA-256 object via sha1=<object-id>. But does it hurt? And it sure would make maintenance less burdensome to leave it as-is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agreed. i don't think we should change it just to be changing it. i'm more worried about someone seeing that line in sha1-file.c and deciding that it needs to be "fixed" somehow -- or if in fact someone already had fixed it and the test suite had insufficient coverage.
Perhaps, we could add a comment in t0410 summarizing this.
Changing the test to be something like =~ /^sha[0-9]+=... might help future-proof us, but then again, we stick the result in a variable called sha1, so we're not really making anything clearer. A comment should be sufficient for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess the best idea would be to change the sha1 (or more likely, add an alias) to hash or even oid.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed the commit into a squash!, with the following paragraph (which is intended to be folded into the target commit verbatim):
The read-object hook feature was designed before the SHA-256 support was
even close to be started. As a consequence, its protocol hard-codes the
key `sha1`, even if we now also support SHA-256 object IDs.
Technically, this is wrong, and probably the best way forward would be
to rename the key to `oid` (or `sha256`, but that is less future-proof).
However, there are existing setups out there, with existing read-object
hooks that most likely have no idea what to do with `oid` requests. So
let's leave the key as `sha1` for the time being, even if it will be
technically incorrect in SHA-256 repositories.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, renaming the variable name to something like oid would be fine. Whatever is easiest -- I'm really not trying to cause extra work here. :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not really a variable. It's a hard-coded part of the read-object protocol, i.e. the text that is sent to the hook to request certain objects.
Right, seems as if the Perforce daemon did not listen anymore at random stages. I am unsure where that comes from, but suspect that a recent update of Perforce itself is responsible for this flakiness. |
…ve missing objects The read-object hook feature was designed before the SHA-256 support was even close to be started. As a consequence, its protocol hard-codes the key `sha1`, even if we now also support SHA-256 object IDs. Technically, this is wrong, and probably the best way forward would be to rename the key to `oid` (or `sha256`, but that is less future-proof). However, there are existing setups out there, with existing read-object hooks that most likely have no idea what to do with `oid` requests. So let's leave the key as `sha1` for the time being, even if it will be technically incorrect in SHA-256 repositories.
Let's prepare for the SHA-256 support in Git that will be shipped as part of v2.29.0.
We need this to prepare for the upcoming SHA-256 support in v2.29.0.
…rge deltas Just another test number conflict in the upcoming v2.29.0. Signed-off-by: Johannes Schindelin <[email protected]>
This is a backport of `ss/cmake-build` so that we can merge it into Git for Windows early. Signed-off-by: Johannes Schindelin <[email protected]>
Since we're using CMake in the CI builds now, we were conveniently notified that the `fscache.c` file needs to be added to the Windows-specific source files. Signed-off-by: Johannes Schindelin <[email protected]>
Signed-off-by: Johannes Schindelin <[email protected]>
Let's also adjust the CMake support to include the gvfs-helper.
Let's adjust the CMake support to accommodate for the GVFS protocol test helper.
0ea1550 to
60b3006
Compare
|
The usual failure in the p4 tests (as I mentioned, I suspect a Perforce upgrade to be the culprit). |
This PR tries to prepare for the upcoming v2.29.0-rc0 (which is due on October 5th, 2020, i.e. 2 weeks from now). Concretely, it: