Skip to content

Conversation

@dscho
Copy link
Member

@dscho dscho commented Sep 21, 2020

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:

  • prepares the read-object hook patches as well as the serialized status patches for the SHA-256 support,
  • changes the number of a test script to make room (that number will be used for a different test in v2.29.0), and
  • pulls in a backported version of the CMake support so that we can adjust our patches that add the GVFS helper and the GVFS protocol test helper.

SibiSiddharthan and others added 8 commits September 18, 2020 10:50
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]>
Copy link

@derrickstolee derrickstolee left a 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.

Copy link

@jeffhostetler jeffhostetler left a 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})$/;

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.

Copy link
Member Author

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":

git/sha1-file.c

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.

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.

Copy link
Member Author

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.

Copy link
Member Author

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.

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. :-)

Copy link
Member Author

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.

@dscho
Copy link
Member Author

dscho commented Sep 22, 2020

I kicked off a new set of actions builds because of some p4-related errors on one macOS build. Not your issue.

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.

dscho and others added 9 commits September 23, 2020 15:43
…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.
@dscho dscho force-pushed the prepare-for-v2.29.0 branch from 0ea1550 to 60b3006 Compare September 23, 2020 13:48
@dscho
Copy link
Member Author

dscho commented Sep 23, 2020

The usual failure in the p4 tests (as I mentioned, I suspect a Perforce upgrade to be the culprit).

@dscho dscho merged commit 8c31fdc into microsoft:vfs-2.28.0 Sep 24, 2020
@dscho dscho deleted the prepare-for-v2.29.0 branch September 24, 2020 08:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants