git-for-windows / git

A fork of Git containing Windows-specific patches.
http://gitforwindows.org/
Other
8.4k stars 2.56k forks source link

VS compilation (of extra code) not picking up zlib when 'gcc make' does #2352

Closed PhilipOakley closed 3 years ago

PhilipOakley commented 5 years ago

Having got the VS studio compilation of vs/master (git.sln) to work (#2349 , #2351 , https://github.com/git-for-windows/build-extra/pull/258) for me, I returned to the 4GiB problem #2179 using my size_t_6rebase branch, on top of vs/master. It includes a test helper to read the zlib compile flags, which is causing me problems in the compile/link stages.

However, I find that my code will 'make' with the SDK/gcc, but fails with Visual Studio, because it reports

19>Generating Code...
19>test-tool.obj : error LNK2001: unresolved external symbol cmd__zlib_compile_flags
19>..\test-tool.exe : fatal error LNK1120: 1 unresolved externals
19>Done building project "test-tool.vcxproj" -- FAILED.
========== Build: 18 succeeded, 1 failed, 0 up-to-date, 0 skipped ==========

I'm unclear where in sln build generator I need to make the connection between the new function and the zlib library.

And pointers to some nuance that I've missed when I added that function, especially for preparing the .sln (maybe I just need to regenerate it? - maybe that's it (need to remember how to do that) but it's late now...)

Setup

as per #2351 Win 10, 64 bit. Git 2.23. with SDK, the VSgit code is in a seperate worktree.

dscho commented 5 years ago

19>test-tool.obj : error LNK2001: unresolved external symbol cmd__zlib_compile_flags

This symbol is not defined in vs/master. Did you maybe just merge the size_t_6rebase branch into vs/master? If so, you may want to run make vcxproj again, or manually add t/helper/test-zlib-compile-flags.c to the test-tool project's source files.

PhilipOakley commented 5 years ago

@dscho thanks for confirming. I was well tired last night;-)

I had rebased (rather than merged) the size_t_6 work (with some earlier fixups) on top of the latest vs/master. I'd just lost the plot on how to rebuild the whole thing, and which things would trip up the unwary (me).

I did need to git reset --hard to clean the odd modified bits (GIT-BUILD-OPTIONS, git-cvsserver, git-send-email, git-svn) which mainly had $VERSION changes, while the GIT-BUILD-OPTIONS had USE_LIBPCRE2, NO_PYTHON & NO_GETTEXT option flips. I guess that was from testing with a regular 'make'.

make vcxproj then did a clean generation of the .sln, which when reloaded, compiled clean in VS2017.

I think there is still a minor confusion in the sequence as to the Version string still has the old g. [strike that] - It's that the make vcproj does a final commit on completion # Commit the whole shebang which means that the g isn't the current HEAD hash (i.e. I need to add a note to the unwary... either to the wiki build page, or the C:\git-sdk-64\usr\src\VisualStudioGit\compat\vcbuild\README, probably the former).

I've not worked out how to start a bash window (rather than a CMD window) that picks up the new version, but my CMD window is running OK (now I've change my install options to the latest recommendation;-).

PhilipOakley commented 5 years ago

@dscho in https://github.com/git-for-windows/git/wiki/Compiling-Git-with-Visual-Studio, it's stated that one can "open the git bash and run the tests". However with the recommended settings (git-only in CMD) the testing is not properly working for me.

Did you do your testing using the third path install option of having the *nix commands on path as well?

I am able to run git version in the cmd window and get git version 2.23.0.windows.1.33.g1541e3db21.MSVC reported back. git status works, while git branch starts with error: cannot spawn less: No such file or directory, but then goes onto give the expected output.

If I cd t then the version changes so I can't easily test the compiled version. Is that what you expected? (remembering that if you were testing the plain vanila git then the msvc compile version and the G-f-W ggc compiled version would have been the same, so [lack of] differences may not have been noticed).

In my head I have a memory that the MSVC compilation didn't include any 'install' capability, so I may be attempting too much at the moment (i.e. investigating the >4GiB code with VS leaving many tricky corners un-proven..).

dscho commented 5 years ago

Did you do your testing using the third path install option of having the *nix commands on path as well?

No, I do not need that, as those *nix commands are in the PATH when you run Git Bash. Are they not, in your case? I.e. what happens if you run less.exe from your Git Bash?

PhilipOakley commented 5 years ago

Hi @dscho my problem is the method of starting bash, and recognising it (having always run the bash only minty window, with it's pleasant colouring and title).

I haven't yet found the correct method of starting bash (or cmd) that still gives the correct (compiled) version string when executed from the /t sub-directory.

The XY problem is how to find the correct config settings that will git add file specifically to the loose objects when file is 4.1GiB. And run from the test harness. Ideally using the VS compilation (for intellisense tracing etc). i.e the same unanswered question from https://public-inbox.org/git/f0c838f0-2f75-2b05-1aeb-3db4743ce89a@iee.org/ from March.

The reason for a loose object is debugging. It does not involve the use of pack files nor crs32, just the zlib and the basic size_t length fixes. The commit test after the add checks for the pack/crc32 fixes.

It feels like the MSVC compilation needs an extra install step, but I can't put my finger on the correct place to resolve the issues (especially if they corrupt the expectations you already have in the code).

So when you say "run Git Bash." are you thinking I'm typing bash into a cmd window and going from there, or using one of the bash shortcuts provided either for the sdk, or the regular G-f-W install, or something else? (I'm not thinking it's those shortcuts, so..)

Here's some output, typed at the cmd window just now.

Microsoft Windows [Version 10.0.17763.805]
(c) 2018 Microsoft Corporation. All rights reserved.

C:\Users\phili>cd \git-sdk-64\usr\src\VisualStudioGit

C:\git-sdk-64\usr\src\VisualStudioGit>git version
git version 2.23.0.windows.1.33.g1541e3db21.MSVC

C:\git-sdk-64\usr\src\VisualStudioGit>cd t

C:\git-sdk-64\usr\src\VisualStudioGit\t>git version
git version 2.23.0.windows.1

C:\git-sdk-64\usr\src\VisualStudioGit\t>cd ..

C:\git-sdk-64\usr\src\VisualStudioGit>bash
root@Philip-Win10:/mnt/c/git-sdk-64/usr/src/VisualStudioGit# git version
git version 2.21.0
root@Philip-Win10:/mnt/c/git-sdk-64/usr/src/VisualStudioGit#

(I'm out almost all day Sunday)

dscho commented 5 years ago

I haven't yet found the correct method of starting bash (or cmd) that still gives the correct (compiled) version string when executed from the /t sub-directory.

Doesn't ../bin-wrappers/git version work for you?

dscho commented 5 years ago

So when you say "run Git Bash." are you thinking I'm typing bash into a cmd window

No. I am thinking about starting Git Bash from the Start Menu.

dscho commented 5 years ago

The XY problem is how to find the correct config settings that will git add file specifically to the loose objects when file is 4.1GiB.

I guess that it is added as a pack file? If so, maybe there is some threshold kicking in? From looking at the documentation, I'd suspect core.bigFileThreshold.

PhilipOakley commented 5 years ago

I haven't yet found the correct method of starting bash (or cmd) that still gives the correct (compiled) version string when executed from the /t sub-directory.

Doesn't ../bin-wrappers/git version work for you?

I wasn't really aware of bin-wrappers/git. It had crossed my field of view occasionally but hadn't really registered, and I hadn't looked. (I see it contains shell scripts without the .sh extension.)

Where does it fit within the normal scheme of things? (does something automatically invoke it?)

So when you say "run Git Bash." are you thinking I'm typing bash into a cmd window

No. I am thinking about starting Git Bash from the Start Menu.

My start menu item (shortcut, also pinned to taskbar, location C:\ProgramData\Microsoft\Windows\Start Menu\Programs\Git) has a target "C:\Program Files\Git\git-bash.exe" --cd-to-home, Startin: %HOMEDRIVE%%HOMEPATH%.

I also have the sdk icon (shortcut, location C:\Users\phili\AppData\Roaming\Microsoft\Internet Explorer\Quick Launch\User Pinned\TaskBar) pinned to my taskbar with target C:\git-sdk-64\git-bash.exe Start in: C:/git-sdk-64/.

... So we have distinct git-bash.exe files which I'm not sure what, if any, difference there is between them (or what those git-bash.exe do specially). Maybe I'm getting lost in too may compatibility layers.

PhilipOakley commented 5 years ago

The XY problem is how to find the correct config settings that will git add file specifically to the loose objects when file is 4.1GiB.

I guess that it is added as a pack file? If so, maybe there is some threshold kicking in? From looking at the documentation, I'd suspect core.bigFileThreshold.

I didn't think it was that, as that is for directly streaming files (according to the man page). However the packfile limit is also something I was thinking about being a factor (which G-f-W sets, though IIUC the test system avoids bu having no global or system configs, I think).

I also suspect (after sleeping on it again) that maybe there is some other default parameter that is being used for a check [for loose vs pack] that is still only 'long' in LLP64, so gets tripped by the 32bit long on Windows.

I thought I'd fixed them all (config variable for 64bit files), (packs appears good and fsck check) but getting the code working under the test system was a whole new ball game (as per that linked unresolved email thread).

PhilipOakley commented 5 years ago

So we have distinct git-bash.exe files which I'm not sure what, if any, difference there is between them (or what those git-bash.exe do specially). Maybe I'm getting lost in too may compatibility layers.

maybe it's linked to https://github.com/git-for-windows/git/issues/1674 - odd coincidence. #leftoverdocs

dscho commented 5 years ago

I wasn't really aware of bin-wrappers/git. It had crossed my field of view occasionally but hadn't really registered, and I hadn't looked. (I see it contains shell scripts without the .sh extension.)

Where does it fit within the normal scheme of things? (does something automatically invoke it?)

The bin-wrappers/ directory is prepended to the PATH when running tests, allowing for a just-built Git to be run in-place without installing it. It is not really designed for you to run Git in-place, but it does work.

My start menu item (shortcut, also pinned to taskbar, location C:\ProgramData\Microsoft\Windows\Start Menu\Programs\Git) has a target "C:\Program Files\Git\git-bash.exe" --cd-to-home, Startin: %HOMEDRIVE%%HOMEPATH%.

I also have the sdk icon (shortcut, location C:\Users\phili\AppData\Roaming\Microsoft\Internet Explorer\Quick Launch\User Pinned\TaskBar) pinned to my taskbar with target C:\git-sdk-64\git-bash.exe Start in: C:/git-sdk-64/.

... So we have distinct git-bash.exe files which I'm not sure what, if any, difference there is between them (or what those git-bash.exe do specially). Maybe I'm getting lost in too may compatibility layers.

Well, the Git Bash executes a regular Git for Windows, while the Git SDK Bash executes in the context of Git for Windows' SDK. The most noticeable difference is that the former does not have access to GCC/Pacman etc while the latter does. Also, the pseudo root is different: try running cygpath -aw / in either. You'll get it.

However the packfile limit is also something I was thinking about being a factor (which G-f-W sets, though IIUC the test system avoids bu having no global or system configs, I think).

I think we set the packfile limit only for 32-bit installations. And yes, while running the test suite, the system config is totally ignored.

I also suspect (after sleeping on it again) that maybe there is some other default parameter that is being used for a check [for loose vs pack] that is still only 'long' in LLP64, so gets tripped by the 32bit long on Windows.

Quite possible. Maybe you can find a GCC knob (or an MSVC one) that will warn about undesired cutting of bit widths? The nastiest such cases that I encountered were when GCC would not complain that an intptr_t was passed as a function parameter of type long (which cuts off its high bits on 64-bit).

PhilipOakley commented 5 years ago

Maybe you can find a GCC knob (or an MSVC one) that will warn about undesired cutting of bit widths? The nastiest such cases that I encountered were when GCC would not complain that an intptr_t was passed as a function parameter of type long (which cuts off its high bits on 64-bit).

Apparently see "clang -Wshorten-64-to-32 is very helpful to spot these problem" thread https://public-inbox.org/git/1467742293.3047.3.camel@gmail.com/ from 2016.. It looks like gcc used to action that flag but not any more.

I've just been going hard on the need to explicitly up cast anything that could be coerced (downcast to basic long/int) (while within 'standard'). These 'implementation defined' aspects are a real 'bugger' for portability. (we've discussed this elsewhere ;-)

The possibility of an MSVC flag is a further task...

dscho commented 5 years ago

Apparently see "clang -Wshorten-64-to-32 is very helpful to spot these problem" thread https://public-inbox.org/git/1467742293.3047.3.camel@gmail.com/ from 2016.. It looks like gcc used to action that flag but not any more.

Oh, that's a good one!

And it refuels my idea of maybe considering to switch to clang. Which would be a bigger project, but it might be well worth it.

PhilipOakley commented 5 years ago

Apparently see "clang -Wshorten-64-to-32 is very helpful

considering to switch to clang.

I was thinking about how I would go about trying the regular git make on G-f-W sdk using CC=clang, but it's too much of a leap just now (to work out what I need to do) - I need to stop digging elsewhere first...

dscho commented 5 years ago

I would go about trying the regular git make on G-f-W sdk using CC=clang

Well, you already got it: make -j$(nproc) DEVELOPER=1 CC=clang. That should do the trick.

Of course, you also need clang (which is not installed in Git for Windows' SDK by default): pacman -S --noconfirm mingw-w64-x86_64-clang (beware, this will download 500MB).

PhilipOakley commented 5 years ago

I would go about trying the regular git make on G-f-W sdk using CC=clang

Well, you already got it: make -j$(nproc) DEVELOPER=1 CC=clang. That should do the trick.

Of course, you also need clang (which is not installed in Git for Windows' SDK by default): pacman -S --noconfirm mingw-w64-x86_64-clang (beware, this will download 500MB).

Magic. Thank you.

(Been chasing my tail with too many other tasks..)

PhilipOakley commented 5 years ago

Of course, you also need clang (which is not installed in Git for Windows' SDK by default): pacman -S --noconfirm mingw-w64-x86_64-clang (beware, this will download 500MB).

Oh, which directory should this be done from? I appears to have a number of pacman directories in different places. I was mainly surprised by the initial question/warning: warning: no /var/cache/pacman/pkg/ cache exists, creating...

Currently it's downloading to C:\git-sdk-64\var\cache\pacman\pkg\mingw-w64-x86_64-clang-8.0.0-2-any.pkg.tar.xz etc (llvm and z3)

dscho commented 5 years ago

Oh, which directory should this be done from?

Any directory you like.

I appears to have a number of pacman directories in different places. I was mainly surprised by the initial question/warning: warning: no /var/cache/pacman/pkg/ cache exists, creating...

That's because you did not initialize the SDK via pacman, you cloned it (and the pacman cache is of course not a part of the git-sdk-64 repository).

Currently it's downloading to C:\git-sdk-64\var\cache\pacman\pkg\mingw-w64-x86_64-clang-8.0.0-2-any.pkg.tar.xz etc (llvm and z3)

:+1:

dscho commented 3 years ago

I am fairly certain that the reported problem is no longer an issue in the current main.