mingw-w64-git: Add ARM64 support - #56
Conversation
3ccff9b to
64ad5cb
Compare
There was a problem hiding this comment.
@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?
64ad5cb to
bdb659c
Compare
Thank you so much for your tireless efforts!
Well, yes and no. Any built-in 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:
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 bet if you look more closely, you will see that the actual
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 |
dscho
left a comment
dscho
left a comment
There was a problem hiding this comment.
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.
bdb659c to
b68b41c
Compare
|
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! |
fe4964e to
d71cfbb
Compare
There was a problem hiding this comment.
@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! 😊
| aarch64) | ||
| SHAREDIR=/clangarm64/share/git | ||
| ;; |
There was a problem hiding this comment.
I wonder whether we can just replace this entire case block with SHAREDIR=/$MINGW_ARCH/share/git. What do you think?
| END | ||
|
|
||
| "MAINICON" ICON "git-for-windows.ico" | ||
| MAINICON ICON "git-for-windows.ico" |
There was a problem hiding this comment.
I tested this both with MINGW_ARCH=mingw64 (GCC) and MINGW_ARCH=clangarm64 (Clang). In both cases, the icon is set correctly:
| then | ||
| pkgname+=("${MINGW_PACKAGE_PREFIX}-${_realname}-pdb") | ||
| options+=('!strip') | ||
| STRIP=llvm-strip |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 <dennis@dennisameling.com>
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 <dennis@dennisameling.com>
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 <dennis@dennisameling.com>
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 <dennis@dennisameling.com>
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 <dennis@dennisameling.com>
d71cfbb to
a352a61
Compare
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 <dennis@dennisameling.com>
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 <dennis@dennisameling.com>
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 <dennis@dennisameling.com>
a352a61 to
36372fc
Compare
|
Alright, I did some updates to the code and tested the following scenarios:
The only small change I did before building was to replace with ... 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 🎉 |
| 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" |
There was a problem hiding this comment.
Indentation in this file is mixed: in some places, tabs are used, and in other places, spaces. Does the team have a preference here?
There was a problem hiding this comment.
In general, I prefer tabs. Thank you for being so diligent!
|
@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 |
dscho
left a comment
dscho
left a comment
There was a problem hiding this comment.
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... 😁).
|
The build worked well enough ;-) Thank you for your contribution, @dennisameling! |
3 participants
Needs git-for-windows/setup-git-for-windows-sdk#397Merged ✅Needs git-for-windows/git#3916Merged ✅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