Closed Bug 1253212 Opened 9 years ago Closed 5 years ago

Add basic Git detection support to MozillaBuild

Categories

(Firefox Build System :: MozillaBuild, task)

Product:
Firefox Build System
Component:
MozillaBuild
Platform:
All
Windows
Type:
task
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: gkw, Assigned: glob)

References

Details

Attachments

(1 file, 7 obsolete files)

bug-1253212_v7.diff
(deleted), patch
away
: review+
Details | Diff | Splinter Review
Attached patch patch v1 (obsolete) (deleted) — — Splinter Review
+++ 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
Attachment #8726150 - Flags: review?(ryanvm)
Please also test on your end, I might not have covered all corners as per the previous LLVM detection bug 1238251.
Assignee: nobody → gary
Status: NEW → ASSIGNED
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.
Attached patch patch v1.1 (obsolete) (deleted) — — Splinter Review
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.
Attachment #8726150 - Attachment is obsolete: true
Attachment #8726150 - Flags: review?(ryanvm)
Attachment #8728751 - Flags: review?(ryanvm)
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.
Flags: needinfo?(gary)
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.
Flags: needinfo?(gary)
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.
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?
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.
OK, I have a revised patch coming up that supports both 32-bit and 64-bit Git, with a caveat.
Summary: Add basic 32-bit Git detection support to MozillaBuild → Add basic Git detection support to MozillaBuild
Attached patch patch v2 (obsolete) (deleted) — — Splinter Review
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
Attachment #8729324 - Flags: review?(ryanvm)
Attachment #8729324 - Flags: feedback?(quanxunzhen)
Comment on attachment 8729324 [details] [diff] [review] patch v2 It at least works for me! Thanks for working on this!
Attachment #8729324 - Flags: feedback?(quanxunzhen) → feedback+
Attachment #8728751 - Attachment is obsolete: true
Attachment #8728751 - Flags: review?(ryanvm)
No longer blocks: MozillaBuild2.2
Why was this removed from blockers of MozillaBuild 2.2? I think this is even more important than LLVM detection...
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 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.
Attachment #8729324 - Flags: review?(ryanvm)
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → WONTFIX
Hmmm... It is unfortunate that MozillaBuild released a new version but doesn't include this.
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?
Flags: needinfo?(ryanvm)
I'll defer to gps on whether he wants to take it. MozillaBuild ownership is on coop's team now.
Flags: needinfo?(ryanvm) → needinfo?(gps)
If we can add Git support easily, we should probably do it.
Flags: needinfo?(gps)
Reopening as per comment 18, and especially if we can ride-along a potential MozillaBuild 3.1.1 fix.
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
Attached patch patch-v3.diff (obsolete) (deleted) — — Splinter Review
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?
Attachment #8729324 - Attachment is obsolete: true
Attachment #8926638 - Flags: review?(ted)
Attachment #8926638 - Flags: feedback?(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.
Attachment #8926638 - Flags: feedback?(ryanvm)
Comment on attachment 8926638 [details] [diff] [review] patch-v3.diff Review-ping from any build folks?
Attachment #8926638 - Flags: review?(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.
Attachment #8926638 - Flags: review?(ted)
Attachment #8926638 - Flags: review?(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.
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.
Assignee: gary → nobody
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.
(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.
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.
It seems running ./mach bootstrap now fails because it can't detect git location.

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

Assignee: nobody → glob
Attached patch bug-1253212_v4.diff (obsolete) (deleted) — — Splinter Review

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
Attachment #8926638 - Attachment is obsolete: true
Comment on attachment 9099236 [details] [diff] [review] bug-1253212_v4.diff David - is this something you're able to review?
Attachment #9099236 - Flags: review?(dmajor)

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.

Flags: needinfo?(glob)

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.

Attached patch bug-1253212_v5.diff (obsolete) (deleted) — — Splinter Review

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
Attachment #9099236 - Attachment is obsolete: true
Attachment #9099236 - Flags: review?(dmajor)
Flags: needinfo?(glob)
Attachment #9099450 - Flags: review?(dmajor)
Attached patch bug-1253212_v6.diff (obsolete) (deleted) — — Splinter Review

Changes since last patch:

Apply fixed token parsing to all REG calls.

Attachment #9099450 - Attachment is obsolete: true
Attachment #9099450 - Flags: review?(dmajor)
Attachment #9099451 - Flags: review?(dmajor)
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?

Yes, that block can just be removed.

Blocks: MozillaBuild3.3
Attached patch bug-1253212_v7.diff (deleted) — — Splinter Review

Changes since last patch:

Remove unnecessary LLVM detection

Attachment #9099451 - Attachment is obsolete: true
Attachment #9099451 - Flags: review?(dmajor)
Attachment #9099615 - Flags: review?(dmajor)
Attachment #9099615 - Flags: review?(dmajor) → review+

Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/mozilla-build/rev/67772d477c94
Add basic Git detection support to MozillaBuild; r=dmajor

Status: REOPENED → RESOLVED
Closed: 8 years ago → 5 years ago
Resolution: --- → FIXED
Product: mozilla.org → Firefox Build System
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: