1253212 - Add basic Git detection support to MozillaBuild

Status: RESOLVED FIXED

(Firefox Build System :: MozillaBuild, task)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: patch v1

+++ This bug was initially created as a clone of Bug #1238251 +++

We have bug 590921 to add Git to MozillaBuild. However, Git can be installed separately via:

https://git-scm.com/download/
https://git-for-windows.github.io/

(or even via the Visual Studio 2015 Community Edition installer)

We could add detection logic to allow us to use basic git.exe commands in MozillaBuild without specifying the full path to it, if Git was installed separately.

Notes:
* Patch works with the latest Git 2.7.2 version, but only the 32-bit one.
** I installed 32-bit Git without any optional components. (Important?)

* Issue with 64-bit Git:
** Somehow somewhere the path to 32-bit Program Files is called even when I uninstalled 32-bit then installed and ran 64-bit Git. However, it might just be my own problem. I cannot guarantee this patch will work with 64-bit Git though.

* I tested an older 1.9.5-preview20150319 release
** This failed to work as it didn't seem to install the required registry keys.

(32-bit and 64-bit Gits were mutually exclusively installed, i.e. when I tested 64-bit after 32-bit, I made sure to uninstall 32-bit first)


Sample output with patch (apply on top of mozilla-build rev 8f49cd85c7ac):

MozillaBuild Install Directory: C:\mozilla-build\
Visual C++ 2013 Directory: C:\Program Files (x86)\Microsoft Visual Studio 12.0\V
C\
Windows SDK Directory: C:\Program Files (x86)\Windows Kits\8.1\
Using the MSVC 2013 64-bit cross-compile toolchain.


$ git version
git version 2.7.2.windows.1

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Please also test on your end, I might not have covered all corners as per the previous LLVM detection bug 1238251.

Comment 2

[Xidorn Quan [:xidorn] UTC+10]

It seems for me the information of git is placed in HKCU\SOFTWARE\GitForWindows, and I'm using Win64. There is a Wow6432Node under HKCU\SOFTWARE as well. So I guess you would need to detect all four places for Win64, and two of them for Win32.

Comment 3

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: patch v1.1

Made just the following change between patch v1 and v1.1:

-  SET PATH=%PATH%;!GITDIR!\bin
+  SET PATH=!PATH!;!GITDIR!\bin

This is needed else start-shell-msvc2013.bat fails to start properly.

Comment 4

[Xidorn Quan [:xidorn] UTC+10]

Would you consider my suggestion in comment 2?

I think you need to detect "{HKLM,HKCU}\SOFTWARE\{,Wow6432Node\}GitForWindows" for Win64. It wouldn't hurt anything to detect all of those for Win32 as well, I suppose.

Comment 5

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

I'm not really familiar on the HKey Local Machine (HKLM) vs HKey Current User (HKCU) differences.

On my test Win7 box, when I install Git 32-bit 2.7.2, I don't seem to find any registry keys installed in HKCU, unless I'm missing something here.

Comment 6

[Xidorn Quan [:xidorn] UTC+10]

Git supports being installed only for the current user only, in which case, the program files goes to %USERPROFILE%\AppData\Local\Programs\Git by default, and the information goes to HKCU rather than HKLM.

Comment 7

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

I didn't see anything in the Git installer off:

https://github.com/git-for-windows/git/releases/download/v2.7.2.windows.1/Git-2.7.2-32-bit.exe

that would allow being "installed only for the current user only". Is there a command-line switch to activate?

Comment 8

[Xidorn Quan [:xidorn] UTC+10]

Hmmm, probably some previous version allows, or some other software install Git that way. I uninstall and reinstall it from the official package, then everything goes to HKLM now. Probably we do not need to support HKCU then.

But I think it still worth supporting 64-bit Git for Win64, especially given the default download from the official site for Win64 is the 64-bit one.

Comment 9

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

OK, I have a revised patch coming up that supports both 32-bit and 64-bit Git, with a caveat.

Comment 10

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: patch v2

The caveat is that when 64-bit Git is installed, typing just "git" throws a seemingly incorrect error:

$ git
bash: /c/Program Files (x86)/Git/bin/git.exe: No such file or directory

Not sure why it calls git from "Program Files (x86)", my PATH only has "/c/Program Files/Git/bin" added. 32-bit Git does not have this issue.

Thus, both 32-bit and 64-bit Git work, if one uses "git.exe".

===

More info:

$ git.exe version
git version 2.7.2.windows.1

$ file /c/Program\ Files/Git/bin/git.exe
/c/Program Files/Git/bin/git.exe: PE32+ executable for MS Windows (console) Mono
/.Net assembly

Comment 11

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8729324 [details] [diff] [review]
patch v2

It at least works for me! Thanks for working on this!

Comment 12

[Xidorn Quan [:xidorn] UTC+10]

Why was this removed from blockers of MozillaBuild 2.2? I think this is even more important than LLVM detection...

Comment 13

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

So there is a bit of a rush to get 2.2 out the door, and RyanVM would like to first test this more.

Thus he poked me on IRC and we agreed this can differ. In the meantime you can use:

/c/Program\ Files/Git/bin/git.exe (64-bit)
/c/Program\ Files\ \(x86\)/Git/bin/git.exe (32-bit)

or something similar, or just apply the patch locally.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8729324 [details] [diff] [review]
patch v2

MSYS1-based MozillaBuild is effectively EOL with gps' summer intern Nat nearly having msys2 operational at this point, so I'm calling this bug WONTFIX.

Comment 15

[Xidorn Quan [:xidorn] UTC+10]

Hmmm... It is unfortunate that MozillaBuild released a new version but doesn't include this.

Comment 16

[Xidorn Quan [:xidorn] UTC+10]

RyanVM, given that we have another bug (bug 1383578), I assume you are going to release a fixup version. Could you include this one in that new version as well?

Comment 17

[Ryan VanderMeulen [:RyanVM]]

I'll defer to gps on whether he wants to take it. MozillaBuild ownership is on coop's team now.

Comment 18

[Gregory Szorc [:gps]]

If we can add Git support easily, we should probably do it.

Comment 19

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Reopening as per comment 18, and especially if we can ride-along a potential MozillaBuild 3.1.1 fix.

Comment 20

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: patch-v3.diff

Same as patch v2, but with fixes for paths as per bug 1383578.

Ryan, would this be able to make it to a potential 3.1.1?

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8926638 [details] [diff] [review]
patch-v3.diff

I'm pretty loathe to add more PATH setting logic to start-shell.bat given the past issues we've had, but if it passes review with the official MozillaBuild maintainers, then sure.

Comment 22

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Comment on attachment 8926638 [details] [diff] [review]
patch-v3.diff

Review-ping from any build folks?

Comment 23

[Gregory Szorc [:gps]]

Comment on attachment 8926638 [details] [diff] [review]
patch-v3.diff

Review of attachment 8926638 [details] [diff] [review]:
-----------------------------------------------------------------

I'm fine with doing this. But please call `UNSET` to wipe out any local variables so they don't get inherited by the MozillaBuild shell. I'm specifically worried about DIR* variables. GIT_DIR is a special variable to Git. And having GITDIR and GITKEY set scares me some.

Comment 24

[Gregory Szorc [:gps]]

Also, I'm assuming someone has tested this somewhat thoroughly. The interaction between any program and the msys environment can be quite finicky. If it works, great. Although I would feel better if there is an escape hatch where people can disable it if it causes problems. Maybe put everything behind an "IF NOT MOZILLABUILD_DISABLE_GIT" block or something.

Comment 25

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

In that case, I'd like to throw this bug back out in the pool. I've spent a little too much total time on this, I'm not sure I'm the right person to take it to the next level.

Comment 26

[Gregory Szorc [:gps]]

The patch was literally a few lines away from an r+.

Does that mean this hasn't been thoroughly tested? If it hasn't we can always make this code optional - behind an "IF MOZILLABUILD_DETECT_GIT" block - and tell people to test with the next MozillaBuild release. If it works, we enable by default. If not, we go back to the drawing board.

Comment 27

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to Gregory Szorc [:gps] from comment #26)
> Does that mean this hasn't been thoroughly tested?

Well, I tested it, I think Xidorn also did, but I didn't get feedback anywhere else. Bug 1383578 and bug 1260823 were bugs that folks filed against the previous LLVM PATH issue, which I've noted to be careful with, in this patch. I've been running locally with my patch for more than 1.5 years now against multiple MozillaBuild revisions.

Whether or not that satisfies other people's usecases, I'm not sure if it passes the "test" to be on by default.

According to the comments and history above, this bug was resolved WONTFIX in July last year, the ownership seemed to pass over to :coop's team, I'm just helping out.

In the meantime, there is another workaround I think - I can set a .bash_aliases in my home "~" directory to my locally installed Git binary and typing "git" should still work.

Comment 28

[Thomas Daede [:TD-Linux]]

Mach bootstrap now needs git, which means the directions on https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Windows_Prerequisites no longer work.

Comment 29

[Xiaoyin Liu]

It seems running ./mach bootstrap now fails because it can't detect git location.

Comment 30

[Tom Ritter [:tjr]]

I used the patch on a Windows 10 x64 installation with Git for Windows 2.23.0 and it worked fine for me.

Comment 31

[:glob ✱]

Attachment: bug-1253212_v4.diff

Taking to push over the finish line - this impacts moz-phab too, which requires git to install dependencies and apply patches.

Updated patch that:

• adds disabling mechanism via MOZ_NO_GIT_DETECT
• unsets GITDIR and GITKEY variables

Comment 32

[:glob ✱]

Comment on attachment 9099236 [details] [diff] [review]
bug-1253212_v4.diff

David - is this something you're able to review?

Comment 33

[(Away)]

Do we need to check HKCU too, per comment 6?

And, to make sure I understand correctly, you want to add git to PATH regardless of whether or not it's in the user's PATH for normal command prompts? (I can't remember whether the installer offers to do this.) Otherwise it would be an easy check for where git before we reset PATH.

Comment 34

[Xiaoyin Liu]

I think it's probably a good idea to check the registry keys (HKLM\SOFTWARE\GitForWindows, etc.) and where git. The Git for Windows installer has an option to keep user's PATH unmodified, but the default one is to add the git.exe directory to PATH. It's also possible that users install other Git clients or use portable Git binary, which do not set the GitForWindows registry keys. By checking both the registry and where git, we can support as many different user's configurations as possible.

Comment 35

[:glob ✱]

Attachment: bug-1253212_v5.diff

Changes since last patch:

Check for git in the following locations (in order):

1. windows path (using where git)
2. HKCU 64-bit
3. HKCU 32-bit
4. HKLM 64-bit
5. HKLM 32-bit

Comment 36

[:glob ✱]

Attachment: bug-1253212_v6.diff

Changes since last patch:

Apply fixed token parsing to all REG calls.

Comment 37

[(Away)]

Comment on attachment 9099451 [details] [diff] [review]
bug-1253212_v6.diff

Review of attachment 9099451 [details] [diff] [review]:
-----------------------------------------------------------------

::: start-shell.bat
@@ +63,5 @@
>  )
>  
> +REM Reset to a known clean path, appending the path to Git if we found it.
> +IF NOT DEFINED MOZ_NO_RESET_PATH (
> +  SET PATH=%SystemRoot%\System32;%SystemRoot%;%SystemRoot%\System32\Wbem

This is going to stomp on the LLVM path from line 62 -- or maybe you are cleverly fixing bug 1547269 at the same time. :) Perhaps we should just remove lines 51-63?

Comment 38

[Ryan VanderMeulen [:RyanVM]]

Yes, that block can just be removed.

Comment 39

[:glob ✱]

Attachment: bug-1253212_v7.diff

Changes since last patch:

Remove unnecessary LLVM detection

Comment 40

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/mozilla-build/rev/67772d477c94
Add basic Git detection support to MozillaBuild; r=dmajor