Win XP (32 bit!) report of successes and issues

[PhilipOakley]

@dscho - I've just done a fresh net install of the latest installer/builder, so here is the reports for what happens on XP. This is a courtesy note.

1. a couple of 'Permission denied' messages.
2. Loads of compile warnings, mainly of 'unknown conversion type character 'l' in format'
3. git status felt to take a long time.
4. git gui reports 'Git directory not found' (so this one feels like a Fail)

So generally a success, with #4# as a small problem.

Downloaded:
https://github.com/git-for-windows/git/releases/download/v2.3.4.windows.2/git-sdk-32-installer-dev

-preview.7z.exe
and ran - got to / past

(158/158) installing mingw-w64-i686-winstoreco... [######################] 100%

"Auto-rebasing .dll files"
mkdir: cannot change permissions of '/dev/shm': Permission denied
mkdir: cannot change permissions of '/dev/mqueue': Permission denied
'C:\WINDOWS\system32\drivers\etc\hosts' -> '/etc/hosts'
'C:\WINDOWS\system32\drivers\etc\protocol' -> '/etc/protocols'
'C:\WINDOWS\system32\drivers\etc\services' -> '/etc/services'
'C:\WINDOWS\system32\drivers\etc\networks' -> '/etc/networks'

Those permission denieds are on C:\git-sdk-32\dev which is set at 'read only'

Mintty came up, clone and compiled, with lots of warnings, generally of the sort
pack-check.c: In function 'verify_packfile':
pack-check.c:114:5: warning: unknown conversion type character 'l' in format [-W format=]
err = error("index CRC mismatch for object %s "
^
pack-check.c:114:5: warning: too many arguments for format [-Wformat-extra-args]
pack-check.c:121:4: warning: unknown conversion type character 'l' in format [-W format=]
err = error("cannot unpack %s from %s at offset %"PRIuMAX"",
^
pack-check.c:121:4: warning: too many arguments for format [-Wformat-extra-args]
[...]

Feeling of slowness... this is the second run.
Philip@PhilipOakley MINGW32 /c/msysgit195 (master)
$ git status
[...]
real 0m2.625s
user 0m0.015s
sys 0m0.109s
(this has git as a sub-module, but I'n not usually here and so it's maybe 'normal')

[PhilipOakley]

https://gist.github.com/PhilipOakley/d8600c5e26de34b11952 has all the mintty clone & compile messages

[PhilipOakley]

Just to say that, for the git gui error, I captured the fleeting dialog before the error message reported above (using camstudio).
The dialog box title bar is for c:\git-sdk-32\mingw32\libexec\git-core\git.exe. So I tried running that from a cmd window, which then gave an error dialog box title 'git.exe Unable To Locate Component' with message "This application has failed to start because liniconv-2.dll was not found. Re-installing the application may fix this problem.", which may, or may not be a clue to what had happened...

[PhilipOakley]

Extra clarification.. The above report is directly on completion of the install. Importantly the CMD window (the one showing the auto-rebasing) never closed, yet the mintty was open as well. On completion of all the above reports, the closure of the bash/mintty (exit) then closed them both.

On restarting via the Git SDK-32 (icon) the git gui now ran, though with lots of windows flashing up and disappearing before the gui actually showed. Tomorrow I'll see if I can capture just how many actually were displayed.

[dscho]

@PhilipOakley thank you for that detailed report.

1. a couple of 'Permission denied' messages.
   [...]
   mkdir: cannot change permissions of '/dev/shm': Permission denied
   mkdir: cannot change permissions of '/dev/mqueue': Permission denied

I would not worry about those: the "cannot change permissions" is expected because MSys2 does not mount the root directory like Cygwin does, with emulated POSIX permissions. This is most likely a bug in the /etc/post-install/01-devices.post file that is installed with upstream's filesystem package: https://github.com/Alexpux/MSYS2-packages/blob/fc568a1a1564edb76ba8a202e917ccf75fcc52f6/filesystem/01-devices.post#L44-L57

1. Loads of compile warnings, mainly of 'unknown conversion type character 'l' in format'

Yeah, I noticed that, too, but there were too many other issues I had to take care of first. After all, these are just warnings ;-)

Jokes aside, I think that this needs to be fixed in that part of the code:

git/compat/mingw.h

Lines 398 to 403 in 521863b

	#ifndef __MINGW64_VERSION_MAJOR
	#define PRIuMAX "I64u"
	#define PRId64 "I64d"
	#else
	#include <inttypes.h>
	#endif

The idea is that the PRI family of constants are defined in /mingw32/i686-w64-mingw32/include/inttypes.h (or in 64-bit setups: /mingw64/x86_64-w64-mingw32/include/inttypes.h). My guess is that something prevents the inttypes.h from defining those constants properly, and as a consequence, this definition kicks in:

git/git-compat-util.h

Lines 243 to 245 in 521863b

	#ifndef PRIuMAX
	#define PRIuMAX "llu"
	#endif

1. git status felt to take a long time.

That is worrisome, but unfortunately half-expected. We had quite a bit of hacks in our patched msys-1.0.dll to accelerate Git's operations. There is even a ticket to look into porting those patches to msys-2.0.dll: #32. Sadly, it turned out that too many changes in Cygwin/MSys2 render that task more Herculean than simply porting patches 😞

Just for the record, could you compare the times it takes to run git status between MSys1 and MSys2 based Git?

1. git gui reports 'Git directory not found' (so this one feels like a Fail)
   [...]
   The dialog box title bar is for c:\git-sdk-32\mingw32\libexec\git-core\git.exe. So I tried running that from a cmd window, which then gave an error dialog box title 'git.exe Unable To Locate Component' with message "This application has failed to start because liniconv-2.dll was not found. Re-installing the application may fix this problem.", which may, or may not be a clue to what had happened...

That is definitely a problem. The libiconv-2.dll message is an indicator, but really a red herring: it suggests that the PATH environment variable contains an incorrect value. The real problem, however, is that something goes wrong with the Git wrapper: To save on space in portable applicatinos, we enhanced the Git wrapper to serve as drop-in replacement for builtins such as git-rev-parse.exe, but there is obviously a problem because Git directory is not correctly set here:

git/git-gui/git-gui.sh

Line 1243 in 521863b

set _gitdir [git rev-parse --git-dir]

when /mingw32/libexec/git-core/git-rev-parse.exe is a copy of the Git wrapper, but it does succeed when it is a copy of git.exe itself.

[dscho]

something goes wrong with the Git wrapper

I boiled it down to the following Tcl code:

puts [exec "c:\\git-sdk-32\\mingw32\\libexec\\git-core\\git-rev-parse.exe" --git-dir]

When running this inside a shell or in cmd.exe, it finds the correct .git/ directory. When running in a wish, it prints an empty string.

The difference of these execution environments is the presence of a Win32 console, of course, so my hunch is that the Git wrapper might do something wrong with stdin/stdout/stderr when it detects that there is no console to attach to.

[dscho]

I boiled it down to the following Tcl code:

puts [exec "c:\\git-sdk-32\\mingw32\\libexec\\git-core\\git-rev-parse.exe" --git-dir]

I fixed the problem locally and am currently finishing up the patches (it was indeed a problem where stdin/stdout/stderr handles were not inherited).

[PhilipOakley]

First start-up of the morning (after shut-down, and captured with camstudio) showed that:

a) there was just the single flash window during the startup from double clicking the icon

b) the initial git status took 6.797s real time, but then only 0.734s on the second run (I don't believe there had been any changes to the Git repo I was stating)

That's all I can manage just now. More later (still to fully rebase the msvc-build as the task I want to get back to ;-)

[dscho]

a) there was just the single flash window during the startup from double clicking the icon

Yep, I saw that, and it is part of the reason why git gui fails to capture the output of git rev-parse --git-dir. As I said, I am fixing up the patches as we write.

b) the initial git status took 6.797s real time, but then only 0.734s on the second run (I don't believe there had been any changes to the Git repo I was stating)

The first time, git status needs to re-index the files to make sure that they have not been altered since the index was written. Then git status writes the new .git/index, including the mtimes of the indexed files. In the second run, the mtimes match with the recorded ones and are older than the mtime of the index itself, so they are not indexed anymore.

I guess that another reason that the first run is slower is that in addition to Git's cache, Windows' own file system cache is still cold.

[PhilipOakley]

@dscho Thanks for the confirmation about the slow cold index issue.
Glad to hear you know what to do about the git gui.
Let's hope Waldek (@weakcamel) has some success with the warnings to lighten your load a bit.
Philip

[dscho]

@PhilipOakley regarding the permission issues, could you test this installer? https://github.com/git-for-windows/build-extra/releases/download/issue-52/git-sdk-32-installer-issue-52.7z.exe (the commit that hopefully works around the problem is this one: git-for-windows/build-extra@25bdd0c)

[PhilipOakley]

I'll give it a go, but it will be tomorrow night. A big canoe / kayaking day tomorrow.

[weakcamel]

Let's hope Waldek (@weakcamel) has some success with the warnings

It took mi browsing 3 repositories to understand how the pieces fit together (net installer vs. SDK vs. git itself) and I finally arrived at the proper destination. I'm on it.

[dscho]

@weakcamel cool. Yeah, things are still a little rough on the edges. The net installer installs the SDK and clones Git and builds it. Welcome to the early days of MSys2-based Git for Windows ;-)

[dscho]

A big canoe / kayaking day tomorrow.

Nice!

I'll give it a go

Please make sure to test git gui again, too, because I believe I addressed that in 5804726, which is already in master (that is in turn built by the net installer).

[weakcamel]

My change has fixed some, but broken some, too - i.e. there are some new warnings in other files. I'll have to look exactly how that's related.

builtin/grep.c
builtin/gc.c
test-chmtime.c

Warnings diff: https://gist.github.com/weakcamel/7ac5f7333923ab3d46a8

BTW, the change fo far:
https://gist.github.com/weakcamel/2f0bc895cb81729af815

I'm not quite sure why 32-bit mingw defines the __MINGW64_VERSION_MAJOR macro...

[dscho]

@weakcamel looks already pretty good!

About those pthread_mutex_init() warnings... I have no idea yet how to solve that problem. pthread_mutex_init() is expected to return non-zero if there was a problem, but our emulation uses InitializeCriticalSection() which returns void. Now, most call sites ignore the return value, but there are two that do not: nedmalloc and thread-utils. I dabbled with the idea of just defining static int pthread_mutex_init(pthread_mutex_t *mutex, const pthread_mutexattr_t *attr) { InitializeCriticalSection(); return 0; }, but maybe something like #define pthread_mutex_init(a, b) (InitializeCriticalSection(a), (b) ? 0 : 0) would do the job, too...

A little more important seems to be this one:

+builtin/gc.c: In function 'lock_repo_for_gc':
+builtin/gc.c:225:4: warning: format '%u' expects argument of type 'unsigned int *', but argument 3 has type 'uintmax_t *' [-Wformat=]
+ fscanf(fp, "%"PRIuMAX" %127c", &pid, locking_host) == 2 && 

It means that PRIuMAX is set to u?

Another question:

-#ifndef __MINGW64_VERSION_MAJOR
+#if !defined(__MINGW64_VERSION_MAJOR) || defined (__MINGW32__)
#define PRIuMAX "I64u"
#define PRId64 "I64d" 

Hmm, I wonder why the lines 107 and 42 of /mingw64/x86_64-w64-mingw32/include/inttypes.h don't do the job. Any idea?

Keep up the good work!

[PhilipOakley]

@dscho SUCCESS; I've tested your trial installer https://github.com/git-for-windows/build-extra/releases/download/issue-52/git-sdk-32-installer-issue-52.7z.exe

• The permission fails have gone
• I like the new message stating what the old window is done (that was 2060 seconds after starting - it takes a while on my home internet ;-)
• the compile warnings are still there (as we'd suspect), @weakcamel is on the case - yeah.
• Both the git gui and gitkworked fine, and I could start each from each other, as well as from the command line, and it worked on restart of the bat file.
• The final command prompt arrived at 4790 secs (just so others know how long to allow for the long coffee break ;-)

minor personal issue was that, as I'd deleted the old C:\git-sdk-32, the desktop shortcut had lost its icon, even after the install. However now that I've double clicked it, it has now recovered (something to add to 'ignore pile').

Summary - SUCCESS
OK to close

[weakcamel]

• Yup, it's:

#define PRIuMAX "I64u"

so the lock_repo_for_gc() has a right to complain (uintmax_t being unsigned unsigned long). It seems a bit overkill, as pid can't really take much than 32 bits, and pid_t is elsewhere defined as basically int.

Perhaps on 32 bits, uintmax_t should not try to be uul at all?

• The reason why it gets overwritten is loading at 301 of

/* Set PRI... and SCN... according to __USE_MINGW_ANSI_STDIO. */
#include <_mingw_print_pop.h>

that takes and re-defines all the constants. Can't say I grasp the logic of that yet.

Ouch. I seem to produce more question than do actual good...

[dscho]

as I'd deleted the old C:\git-sdk-32, the desktop shortcut had lost its icon, even after the install. However now that I've double clicked it, it has now recovered (something to add to 'ignore pile').

It is not always clear to me how Windows refreshes the icon, but I think in your case only the double-click refreshed it, not the re-installation. I agree with you that we should not worry about this too much.

[dscho]

@weakcamel

Yup, it's:

#define PRIuMAX "I64u"

Hmm, from the warning "builtin/gc.c:225:4: warning: format '%u' expects argument of type 'unsigned int *', but argument 3 has type 'uintmax_t *'" I would have expected that PRIuMAX is set to u (instead of I64u) at this point.

The easiest way to find out what value PRIuMAX is #defined to have in this line is to recompile the file with GCC's -E flag ("pre-process only"). I usually run something like touch builtin/gc.c && make V=1 builtin/gc.o to copy the exact invocation, then change the -c to -E and the builtin/gc.o to builtin/gc.E after pasting the command, before hitting Enter. The resulting builtin/gc.E looks quite a bit intimidating because all of the #includes are inlined now, and every once in a while you will see a pre-processor statement mentioning original file names and line numbers. To find my way through this haystack, I typically open the file in a proper editor and let it search for a specific string I suspect to be unique (or at least unique enough to find the correct line quickly), such as "pid, lock_string".

so the lock_repo_for_gc() has a right to complain (uintmax_t being unsigned unsigned long).

Actually, I do not understand why it complains. The fscanf() call gets the appropriate format string for uintmax_t and the &pid parameter is indeed a pointer to an uintmax_t.

It seems a bit overkill, as pid can't really take much than 32 bits, and pid_t is elsewhere defined as basically int.

The point of using uintmax_t is to be on the safe side and not make any assumptions about the exact pid_t type... After all, there is no PRIpid_t constant ;-)

Perhaps on 32 bits, uintmax_t should not try to be uul at all?

Actually, uintmax_t is the largest integer type that the current compiler can handle. We need to trust the header that comes with the compiler to know better than ourselves what that type is ;-)

The reason why it gets overwritten is loading at 301 of

/* Set PRI... and SCN... according to __USE_MINGW_ANSI_STDIO. */
#include <_mingw_print_pop.h>

that takes and re-defines all the constants. Can't say I grasp the logic of that yet.

Oh... Maybe we have to #define __USE_MINGW_ANSI_STDIO 0 somewhere?

[weakcamel]

Thanks a lot for the explanation and tips..
I've been in separation with compiled languages for 3 years; it's coming back.. slowly ;-)

I've tested with __USE_MINGW_ANSI_STDIO disabled and it worked well. Do I understand correctly:

• we should have it disabled only on mingw32 and on mingw64 use the ANSI_STDIO?
• preferred option would be to disable it in mingw.h than in makefile (e.g. basing on $MSYSTEM) ? Or are there plans to use autotools at some point?

Hmm, from the warning "builtin/gc.c:225:4: warning: format '%u' expects argument of type 'unsigned int *', but argument 3 has type 'uintmax_t *'" I would have expected that PRIuMAX is set to u (instead of I64u) at this point.

It is I64u there:

224 "builtin/gc.c"

time(((void *)0)) - st.st_mtime <= 12 * 3600 &&
fscanf(fp, "%""I64u"" %127c", &pid, locking_host) == 2 &&

I found a bug report for scanf on mingw... and then lost it.

BTW, it should probably read in SCNuMAX and not PRIuMAX, not that it makes much difference (the same macro value).

[dscho]

I've tested with __USE_MINGW_ANSI_STDIO disabled and it worked well. Do I understand correctly:

• we should have it disabled only on mingw32 and on mingw64 use the ANSI_STDIO?

Does it work on mingw64, too? If so, it would be better not to special-case it.

preferred option would be to disable it in mingw.h than in makefile (e.g. basing on $MSYSTEM) ?

I think the constant has to be set in mingw.h to make sure that it is set before #includeing stdio.h and inttypes.h. And if it works for both 32-bit and 64-bit, as I said above, I'd like to avoid special-cases.

Or are there plans to use autotools at some point?

Heh. There is a configure.ac, and it was used by MSys2. But the truth is that in the Git for Windows project, we already know what we want to include, and using ./configure would only run the risk that important parts are not built (such as HTTP support, if cURL was not yet installed). Traditionally, Git's build is configured through config.mak.uname, so we switched back to the same way upstream handles things.

it should probably read in SCNuMAX and not PRIuMAX

True. We probably need to augment these lines, too.

[dscho]

@weakcamel I actually managed to silence all warnings in this branch: dscho/git@git-for-windows:master...squelch-warnings

Do you still want to fix the SCNuMAX issue?

[dscho]

(Note: I just realized I had forgotten a warning in regcomp.c... still on it.)

[dscho]

Note: the regcomp.c warnings are gone now (in said branch).

[weakcamel]

Do you still want to fix the SCNuMAX issue?

@dscho Sure, thanks; here: #60

[dscho]

Thanks, @weakcamel!

[dscho]

@PhilipOakley would you be satisfied enough with git gui working and the permission errors and the warnings being gone? I mean, satisfied enough to close this issue? (The remaining issue -- the performance -- should probably be discussed in #48 and #32 instead...)

[PhilipOakley]

Yes @dscho it's good to close. (just wanted confirmation there wasn't something else you wanted noted ;-). Closing now.

[kblees]

Just for the record, could you compare the times it takes to run git status between MSys1 and MSys2 based Git?

MSys1 (built from msysgit/git/master, i.e. v2.1.0 32-bit): 0.756s
MSys2 (v2.3.5.windows.1 64-bit): 0.654s