mingw-w64-git: Add ARM64 support #56

dennisameling · 2022-09-17T16:07:43Z

~~Needs git-for-windows/setup-git-for-windows-sdk#397~~ Merged ✅
~~Needs git-for-windows/git#3916~~ Merged ✅
Needs #59
Needs #58

Here's a successful CI run where I built this with MINGW_ARCH=clangarm64 makepkg-mingw: https://github.com/dennisameling/git/actions/runs/3119954673/jobs/5060230321

dennisameling

@dscho, this is ready for review now 😊

A more general question: the generated built-ins are very large (git-add.exe, git-am.exe, etc.). Can I use SKIP_DASHED_BUILT_INS=YesPlease or are the built-ins still expected by anything in Git? I saw that your build-artifacts pipeline suffers from the same issue: the binaries in the pkg-x86_64 artifact are very large. But then in the resulting portable-x86_64 artifact, the built-ins are magnitudes of times smaller (e.g. git-add.exe is 3.497 kB in pkg-x86_64 but only 34 kb in portable-x86_64). Looks like some magic linking process takes care of that - would you mind elaborating how that works?

mingw-w64-git/PKGBUILD

mingw-w64-git/git-bash.rc

dscho · 2022-09-26T06:58:10Z

@dscho, this is ready for review now 😊

Thank you so much for your tireless efforts!

A more general question: the generated built-ins are very large (git-add.exe, git-am.exe, etc.).

Well, yes and no. Any built-in .exe looks very large on its own, especially when including debug information. But every built-in is a hard-link of git.exe (we even had to document this in the Known Issues section of Git for Windows' release notes).

If you look at the Pacman package in https://github.com/git-for-windows/git-sdk-64/actions/runs/3117023895, for example, you will see this:

Can I use SKIP_DASHED_BUILT_INS=YesPlease or are the built-ins still expected by anything in Git?

I've dragged my feet on "flipping the switch" and just go ahead to skip dashed built-ins in Git for Windows altogether. It is probably not that high a risk, but any sweeping change bears a risk.

Maybe it is time, though. Once v2.38.0 is released (slated for Oct 3rd/4th), I am willing to make the jump.

I saw that your build-artifacts pipeline suffers from the same issue: the binaries in the pkg-x86_64 artifact are very large.

I bet if you look more closely, you will see that the actual mingw-w64-x86_64-git package is relatively small because it uses hard links. The pkg-x86_64 artifact's size is dominated by the source package, which is large because it contains the entire git-for-windows/git repository.

But then in the resulting portable-x86_64 artifact, the built-ins are magnitudes of times smaller (e.g. git-add.exe is 3.497 kB in pkg-x86_64 but only 34 kb in portable-x86_64). Looks like some magic linking process takes care of that - would you mind elaborating how that works?

Due to hard-won experience, we expect users to miss the fact that Git is not really large but that the overall size is dramatically smaller than the sum of the individual file sizes (because many of them are hard links). So, even if it means a slight start-up cost and an extra process in the (unlikely) case that a dashed built-in is executed, we replace the built-ins with copies of the Git wrapper (which essentially modifies the command-line and then spawns the actual git.exe).

dscho

This looks pretty good, and obviously took an enormous amount of effort. Thank you so much!

I would like to ask for just a few more changes, in particular for commit messages inspired by https://github.blog/2022-06-30-write-better-commits-build-better-projects/.

Also note that I will probably not integrate any PR until Oct 3rd unless it is critical to v2.38.0, but will be happy to start merging left and right after that date.

mingw-w64-git/PKGBUILD

mingw-w64-git/git-bash.rc

dennisameling · 2022-09-28T23:28:15Z

Thanks for all the feedback! I've split the work into several commits with (hopefully clear enough) descriptions. It's in a much better state than it was already, but I'll need to do some optimizations here and there. Will ping you when this is ready to review again!

dennisameling

@dscho here we go! Think we're in much better shape now in terms of commits. I tried building this with MINGW_ARCH=mingw64 and MINGW_ARCH=clangarm64. Both are building correctly 👍🏼

Thanks in advance again for reviewing the changes. Let's see if we can get this PR in an approved state soon, so that it can be merged right after the 2.38.0 release on October 3rd/4th. Exciting progress!

we replace the built-ins with copies of the Git wrapper

That was exactly the hint I needed. I was wondering why my portable demo still had those large binaries while the ones from your git-artifacts were tiny. Looks like I'll need to update git-extra.install.in to add clangarm64! 😊

dennisameling · 2022-09-29T09:43:31Z

mingw-w64-git/PKGBUILD

+aarch64)
+  SHAREDIR=/clangarm64/share/git
+  ;;


I wonder whether we can just replace this entire case block with SHAREDIR=/$MINGW_ARCH/share/git. What do you think?

dennisameling · 2022-09-29T09:49:20Z

mingw-w64-git/git-bash.rc

@@ -4,4 +4,4 @@ BEGIN
 	1 "SHOW_CONSOLE=1 APPEND_QUOTE=1 @@COMSPEC@@ /S /C """"@@EXEPATH@@\\usr\\bin\\bash.exe"" --login -i"
 END

-"MAINICON" ICON "git-for-windows.ico"
+MAINICON ICON "git-for-windows.ico"


I tested this both with MINGW_ARCH=mingw64 (GCC) and MINGW_ARCH=clangarm64 (Clang). In both cases, the icon is set correctly:

mstorsjo · 2022-10-04T19:29:34Z

mingw-w64-git/PKGBUILD


 if [[ "$MSYSTEM" == CLANG* ]]
 then
  if test -z "$WITHOUT_PDBS"
  then
    pkgname+=("${MINGW_PACKAGE_PREFIX}-${_realname}-pdb")
    options+=('!strip')
+    STRIP=llvm-strip


One sentence in the commit message is a bit mixed up/misleading; LLVM based toolchains do have a tool named llvm-strip, but it's not the llvm-strip tool that generates the pdb files, it's the lld linker.

Good catch, thanks! Have updated the commit messages.

When trying to compile .rc files with llvm-rc, the command failed with: llvm-rc: Error parsing file: expected int or identifier, got "MAINICON" It looks like llvm-rc doesn't support quotes here. Removing the quotes results in a successful build and correctly assigns the icon to the resulting binaries. Signed-off-by: Dennis Ameling <[email protected]>

Currently, USE_ASCIIDOCTOR is hardcoded to "YesPlease". However, there are some scenarios where this doesn't work, e.g. for CLANGARM64 which doesn't support ruby yet. As Asciidoctor depends on ruby, this will not work. In a follow-up commit, specific logic for CLANGARM64 will be added to override this value. Signed-off-by: Dennis Ameling <[email protected]>

In preparation for more architecture- and compiler-specific logic, we want to make COMPAT_CFLAGS better configurable. By defaulting to an empty value, we can more easily override it in specific cases. Signed-off-by: Dennis Ameling <[email protected]>

In contrast to GCC, we don't need ${MINGW_PACKAGE_PREFIX}-cv2pdb when compiling mingw-w64-git using a Clang/LLVM-based toolchain. The LLD linker can create .pdb files natively. This block also serves as a foundation for more Clang/LLVM-related logic that we intend to add in follow-up commits. Signed-off-by: Dennis Ameling <[email protected]>

When compiling Git with GCC, we rely on an external tool called cv2pdb-strip to strip the binaries after compilation. The Clang/LLVM toolchain, however, has the llvm-strip command available out of the box which we can leverage when building with this toolchain. Signed-off-by: Dennis Ameling <[email protected]>

We can ask the LLD linker to generate PDBs for us. This commit makes LDFLAGS configurable and sets the configuration for the Clang toolchain. As some of the builtins are hard linked to git-remote-http, LLD doesn't generate PDBs for them. By hard linking them ourselves, we ensure that mingw-w64-git-pdb can be built correctly. Signed-off-by: Dennis Ameling <[email protected]>

In previous commits, we've done preparations to support the Clang compiler and make more things configurable. We're now in a good position to allow builds for CLANGARM64, which can be kicked off as follows from an ARM64 device: MINGW_ARCH=clangarm64 makepkg-mingw Signed-off-by: Dennis Ameling <[email protected]>

In a previous commit, we introduced the ARM64 MSYSTEM to Git for Windows. The MSYS2 team has recently released their official CLANGARM64 MSYSTEM, so we can leverage that to stay consistent with their naming. Signed-off-by: Dennis Ameling <[email protected]>

dennisameling · 2022-10-29T14:46:02Z

Alright, I did some updates to the code and tested the following scenarios:

MINGW_ARCH=clangarm64 makepkg-mingw using git-sdk-64 (ensured that mingw-w64-clang-aarch64-toolchain was installed beforehand) on an ARM64 device
MINGW_ARCH=mingw64 makepkg-mingw using git-sdk-64 on a x64 device
MINGW_ARCH=mingw32 makepkg-mingw using git-sdk-32 on a x64 device

The only small change I did before building was to replace

source=("${_realname}"::"git+https://github.com/git-for-windows/git.git#tag=v$tag"

with

source=("${_realname}"::"git+https://github.com/git-for-windows/git.git"

... so that my recently merged commit was included for building CLANGARM64.

All builds succeeded! Looks like we're in good shape with this PR now 🎉

dennisameling · 2022-10-29T14:57:31Z

mingw-w64-git/PKGBUILD

+if [[ "$MSYSTEM" == CLANG* && -z "$WITHOUT_PDBS" ]]
+then
+  pkgname+=("${MINGW_PACKAGE_PREFIX}-${_realname}-pdb")
+  options+=('!strip')
+  STRIP=llvm-strip
+  STRIP_OPTS=--strip-debug
+  LDFLAGS="-Wl,-pdb=" # Ensures that Clang generates PDB files
+elif test -z "$WITHOUT_PDBS"


Indentation in this file is mixed: in some places, tabs are used, and in other places, spaces. Does the team have a preference here?

In general, I prefer tabs. Thank you for being so diligent!

dennisameling · 2022-11-29T13:35:43Z

@dscho mind taking a look at this one? Would be nice to have this in place so I can more easily continue my work on the various build-extra components. Thanks 🙏🏼

dscho

Excellent!

I just want to test-build before merging, so I'll use this excuse to teach https://github.com/git-for-windows/git-for-windows-automation/ to build MINGW-packages, too. Shouldn't be long (famous last words... 😁).

mingw-w64-git/PKGBUILD

dscho · 2022-11-29T22:51:01Z

The build worked well enough ;-)

Thank you for your contribution, @dennisameling!

dennisameling force-pushed the windows-arm64 branch 3 times, most recently from 3ccff9b to 64ad5cb Compare September 24, 2022 22:33

dennisameling changed the title ~~[WIP] Add ARM64 support~~ Add ARM64 support Sep 24, 2022

dennisameling marked this pull request as ready for review September 24, 2022 22:36

dennisameling commented Sep 24, 2022

View reviewed changes

mingw-w64-git/PKGBUILD Outdated Show resolved Hide resolved

mingw-w64-git/git-bash.rc Outdated Show resolved Hide resolved

dennisameling changed the title ~~Add ARM64 support~~ mingw-w64-git: Add ARM64 support Sep 24, 2022

This was referenced Sep 24, 2022

mingw-w64-git-cv2pdb: add ARM64 support git-for-windows/build-extra#435

Closed

portable + installer: add CLANGARM64 support git-for-windows/build-extra#437

Merged

dennisameling force-pushed the windows-arm64 branch from 64ad5cb to bdb659c Compare September 25, 2022 12:24

dennisameling mentioned this pull request Sep 25, 2022

Tracking issue for Windows ARM64 support git-for-windows/git#3107

Closed

dscho requested changes Sep 26, 2022

View reviewed changes

mingw-w64-git/PKGBUILD Outdated Show resolved Hide resolved

mingw-w64-git/PKGBUILD Show resolved Hide resolved

mingw-w64-git/PKGBUILD Outdated Show resolved Hide resolved

mingw-w64-git/PKGBUILD Outdated Show resolved Hide resolved

mingw-w64-git/git-bash.rc Outdated Show resolved Hide resolved

dennisameling mentioned this pull request Sep 28, 2022

Add ARM64/aarch64 support git-for-windows/setup-git-for-windows-sdk#397

Merged

dennisameling force-pushed the windows-arm64 branch from bdb659c to b68b41c Compare September 28, 2022 23:26

dennisameling force-pushed the windows-arm64 branch 4 times, most recently from fe4964e to d71cfbb Compare September 29, 2022 09:25

dennisameling commented Sep 29, 2022

View reviewed changes

dennisameling requested a review from dscho September 29, 2022 09:51

mstorsjo reviewed Oct 4, 2022

View reviewed changes

dennisameling added 5 commits October 29, 2022 13:20

dennisameling force-pushed the windows-arm64 branch from d71cfbb to a352a61 Compare October 29, 2022 12:12

dennisameling added 2 commits October 29, 2022 15:39

dennisameling force-pushed the windows-arm64 branch from a352a61 to 36372fc Compare October 29, 2022 13:40

dennisameling commented Oct 29, 2022

View reviewed changes

dscho approved these changes Nov 29, 2022

View reviewed changes

mingw-w64-git/PKGBUILD Show resolved Hide resolved

dscho merged commit a7fe9e7 into git-for-windows:main Nov 29, 2022

dennisameling deleted the windows-arm64 branch November 29, 2022 23:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

mingw-w64-git: Add ARM64 support #56

mingw-w64-git: Add ARM64 support #56

dennisameling commented Sep 17, 2022 •

edited

Loading

dennisameling left a comment •

edited

Loading

dscho commented Sep 26, 2022

dscho left a comment

dennisameling commented Sep 28, 2022

dennisameling left a comment •

edited

Loading

dennisameling Sep 29, 2022

dennisameling Sep 29, 2022

mstorsjo Oct 4, 2022

dennisameling Oct 29, 2022

dennisameling commented Oct 29, 2022 •

edited

Loading

dennisameling Oct 29, 2022

dscho Dec 7, 2022

dennisameling commented Nov 29, 2022

dscho left a comment

dscho commented Nov 29, 2022

mingw-w64-git: Add ARM64 support #56

mingw-w64-git: Add ARM64 support #56

Conversation

dennisameling commented Sep 17, 2022 • edited Loading

dennisameling left a comment • edited Loading

Choose a reason for hiding this comment

dscho commented Sep 26, 2022

dscho left a comment

Choose a reason for hiding this comment

dennisameling commented Sep 28, 2022

dennisameling left a comment • edited Loading

Choose a reason for hiding this comment

dennisameling Sep 29, 2022

Choose a reason for hiding this comment

dennisameling Sep 29, 2022

Choose a reason for hiding this comment

mstorsjo Oct 4, 2022

Choose a reason for hiding this comment

dennisameling Oct 29, 2022

Choose a reason for hiding this comment

dennisameling commented Oct 29, 2022 • edited Loading

dennisameling Oct 29, 2022

Choose a reason for hiding this comment

dscho Dec 7, 2022

Choose a reason for hiding this comment

dennisameling commented Nov 29, 2022

dscho left a comment

Choose a reason for hiding this comment

dscho commented Nov 29, 2022

dennisameling commented Sep 17, 2022 •

edited

Loading

dennisameling left a comment •

edited

Loading

dennisameling left a comment •

edited

Loading

dennisameling commented Oct 29, 2022 •

edited

Loading