portable + installer: add CLANGARM64 support #437

dennisameling · 2022-09-24T22:46:09Z

~~Needs git-for-windows/MINGW-packages#56~~ ✅ merged
~~Needs git-for-windows/MINGW-packages#59~~ ✅ merged
~~Needs git-for-windows/MINGW-packages#58~~ ✅ merged
~~Needs git-for-windows/setup-git-for-windows-sdk#397~~ ✅ merged
~~Needs git-for-windows/git#3916~~ ✅ merged
~~Needs msys2/MINGW-packages#13787~~ ✅ merged
~~Needs git-for-windows/MINGW-packages#57~~ ✅ merged
~~Needs #440~~ ✅ merged

How to test

On an ARM64 device, do the following:

Install the git-sdk-64
Make sure you create the clangarm64 folder and start the SDK with MSYSTEM=clangarm64, as well as install tools like the Clang compiler. Note that the Git SDK GitHub Action will do that for you soon - see https://github.com/git-for-windows/setup-git-for-windows-sdk/pull/397/files for all details, including a list of the dependencies you need.
Download this ZIP file below with the Git artifacts: git-artifacts-zst.zip (created with mingw-w64-git: Add ARM64 support MINGW-packages#56)
Download this ZIP file with the curl/openssl/xpdf-tools/wintoast dependencies for clangarm64: git-build-extra-artifacts-zst.zip

Then run the following command for portable:

/usr/src/build-extra/please.sh make_installers_from_mingw_w64_git --version=2.39.0 --portable -o artifacts \
--pkg=/mingw-w64-clang-aarch64-git-2.39.0.1.de9501c66f-1-any.pkg.tar.zst \
--pkg=/mingw-w64-clang-aarch64-git-doc-html-2.39.0.1.de9501c66f-1-any.pkg.tar.zst \
--pkg=/mingw-w64-clang-aarch64-xpdf-tools-4.00-1-any.pkg.tar.zst \
--pkg=/mingw-w64-clang-aarch64-curl-7.86.0-1-any.pkg.tar.zst \
--pkg=/mingw-w64-clang-aarch64-openssl-1.1.1.s-1-any.pkg.tar.zst \
--pkg=/mingw-w64-clang-aarch64-wintoast-1.0.0.181.9b0663d-1-any.pkg.tar.zst

... or this command for installer:

/usr/src/build-extra/please.sh make_installers_from_mingw_w64_git --version=2.39.0 --installer -o artifacts \
--pkg=/mingw-w64-clang-aarch64-git-2.39.0.1.de9501c66f-1-any.pkg.tar.zst \
--pkg=/mingw-w64-clang-aarch64-git-doc-html-2.39.0.1.de9501c66f-1-any.pkg.tar.zst \
--pkg=/mingw-w64-clang-aarch64-xpdf-tools-4.00-1-any.pkg.tar.zst \
--pkg=/mingw-w64-clang-aarch64-curl-7.86.0-1-any.pkg.tar.zst \
--pkg=/mingw-w64-clang-aarch64-openssl-1.1.1.s-1-any.pkg.tar.zst \
--pkg=/mingw-w64-clang-aarch64-wintoast-1.0.0.181.9b0663d-1-any.pkg.tar.zst

rimrul · 2022-09-26T12:01:52Z

error: target not found: mingw-w64-clang-aarch64-antiword

This one will probably compile fine for clangarm64, msys2 seems to compile it already with clang and ubuntu/debian seem to compile it for arm64 with no arch specific patches.

error: target not found: mingw-w64-clang-aarch64-odt2txt

This one will probably need some changes regarding this patch
and it's counterpart in PKGBUILD.

error: target not found: mingw-w64-clang-aarch64-xpdf-tools

xpdf-tools, that's from https://github.com/git-for-windows/MINGW-packages/blob/main/mingw-w64-xpdf, we'd need our own CLANGARM64 repo for that.

dennisameling · 2022-10-29T10:08:39Z

@rimrul I was able to get all the deps to work.

connect, antiword, odt2txt: Enable more clangarm64 packages (connect, antiword, odt2txt) msys2/MINGW-packages#13787
xpdf-tools: the download URL of that package seems to be broken. After fixing it, I was able to build the package correctly locally: mingw-w64-xpdf: fix source URL MINGW-packages#57

Will continue work on this PR soon. It's currently in a "very rough but sort of working" state 😅

dennisameling

@dscho @rimrul This is now ready for a first round of reviews. I was able to generate working portable + installer artifacts for 2.38.1 with this setup 🎉

Curious to hear your thoughts on my general approach here. Especially make-file-list.sh was a bit tricky, but everything is looking good in my testing so far.

Thanks in advance!

dennisameling · 2022-10-30T19:08:43Z

portable/release.sh

-ARCH="$(uname -m)"
-case "$ARCH" in
-i686)
+case "$MSYSTEM" in
+MINGW32)
 	BITNESS=32
+	ARCH=i686
+	ARTIFACT_SUFFIX=32
 	MD_ARG=128M
 	;;
-x86_64)
+MINGW64)
+	BITNESS=64
+	ARCH=x86_64
+	ARTIFACT_SUFFIX=64
+	MD_ARG=256M
+	;;
+CLANGARM64)
 	BITNESS=64
+	ARCH=aarch64
+	ARTIFACT_SUFFIX=ARM64
 	MD_ARG=256M
 	;;
 *)
-	die "Unhandled architecture: $ARCH"
+	die "Unhandled MSYSTEM: $MSYSTEM"
 	;;
 esac
+MSYSTEM_LOWER=${MSYSTEM,,}


$MSYSTEM doesn't seem to be defined in Git for Windows' CI. Is that intentional? Do you have another suggestion on how to handle this? Note that uname -m doesn't work as we build CLANGARM64 using git-sdk-64 (which is x86_64).

Added the environment variable in 17a3a4a

The MSYSTEM variable is defined either in /etc/profile or in git-bash.exe, depending on the exact scenario, or in the case of CI runs by the setup-git-for-windows-sdk Action. It should not be necessary to set it yourself unless it is set incorrectly.

I suspect that defining this incorrectly is responsible for this breakage: Note how the 32-bit build talks about wanting to install an x86_64 package? It should detect the presence of /var/lib/pacman/local/mingw-w64-i686-git-[1-9]*/files, but instead looks for /var/lib/pacman/local/mingw-w64-x86_64-git-[1-9]*/files which isn't there, so it tries to build mingw-w64-git, which is not supported anymore in the 32-bit version of Git for Windows' SDK.

Here's an example of when it failed without me explicitly setting the MSYSTEM in CI: https://github.com/git-for-windows/build-extra/actions/runs/3357168162/jobs/5562796433. The build-artifacts and sdk-artifacts CI jobs don't use setup-git-for-windows-sdk so that explains why it failed. If you want, I can set MSYSTEM: mingw64 only to the build-artifacts and sdk-artifacts CI jobs, or we should use setup-git-for-windows-sdk there too.

No, setting MSYSTEM should be scoped to the particular SDK instance. In other words, here, here and here, we need to insert a line like this (in a separate, preparatory commit):

echo "MSYSTEM=MINGW${{matrix.bitness}}" >>$GITHUB_ENV &&

(The first instance is not inside a matrix, it needs to hard-code MINGW64.)

dennisameling · 2022-11-30T02:44:58Z

make-file-list.sh

-	-e '^/usr/include/' -e '^/mingw../include/' \
+	-e '^/(mingw|clang(arm)?)../bin/.*zstd\.exe$' \
+	-e '^/(mingw|clang(arm)?)../share/doc/openssl/' \
+	-e '^/(mingw|clang(arm)?)../share/doc/gettext/' \


@dscho after some testing, I found that this capturing group (or the non-capturing group alternative: '^/(?:mingw|clang(arm)?)../share/doc/gettext/') only works when -P is specified in grep:

Regexp selection and interpretation: -E, --extended-regexp PATTERN is an extended regular expression (ERE) -F, --fixed-strings PATTERN is a set of newline-separated strings -G, --basic-regexp PATTERN is a basic regular expression (BRE) -P, --perl-regexp PATTERN is a Perl regular expression -e, --regexp=PATTERN use PATTERN for matching -f, --file=FILE obtain PATTERN from FILE -i, --ignore-case ignore case distinctions -w, --word-regexp force PATTERN to match only whole words -x, --line-regexp force PATTERN to match only whole lines -z, --null-data a data line ends in 0 byte, not newline

However, when I tested with -P, I ran into the following error:

grep: the -P option only supports a single pattern

🤦🏼‍♂️ this is slightly getting out of hand.

Would it also be ok for you if I simply add the MSYSTEM_LOWER variable to the pattern, like this:

-e "^/$MSYSTEM_LOWER/share/doc/gettext/" \

... or do you really need mingw.. so that it matches both mingw32 and mingw64? Just checking!

I guess that $MSYSTEM_LOWER would work. Except in the case of clangarm64 where we expect to also have to handle mingw64... But why not simply -e "^/$mingw\|clang$[^/]*/share/doc/gettext/", i.e. match any top-level directory starting either with mingw or with clang?

That works. I compared the output of the "before" and "after" make-file-list.sh and they're the exact same after this change. Thanks!

dennisameling · 2022-12-06T00:58:08Z

@dscho This is ready for review. The build-installers 32 CI job is failing due to a failed PGP check, but all the other ones have succeeded 👍🏼

installer/install.iss

dscho · 2022-12-06T13:34:13Z

installer/release.sh

-ARCH="$(uname -m)"
-
-case "$ARCH" in
-i686)


I would prefer the change from uname -m to $MSYSTEM to be done in a separate commit that still only handles i686 and x86_64, and add the aarch64 part in a separate commit.

Please let me know if you want assistance with that.

dscho · 2022-12-06T13:41:43Z

portable/release.sh

-ARCH="$(uname -m)"
-case "$ARCH" in
-i686)
+case "$MSYSTEM" in
+MINGW32)
 	BITNESS=32
+	ARCH=i686
+	ARTIFACT_SUFFIX=32
 	MD_ARG=128M
 	;;
-x86_64)
+MINGW64)
+	BITNESS=64
+	ARCH=x86_64
+	ARTIFACT_SUFFIX=64
+	MD_ARG=256M
+	;;
+CLANGARM64)
 	BITNESS=64
+	ARCH=aarch64
+	ARTIFACT_SUFFIX=ARM64
 	MD_ARG=256M
 	;;
 *)
-	die "Unhandled architecture: $ARCH"
+	die "Unhandled MSYSTEM: $MSYSTEM"
 	;;
 esac
+MSYSTEM_LOWER=${MSYSTEM,,}


No, setting MSYSTEM should be scoped to the particular SDK instance. In other words, here, here and here, we need to insert a line like this (in a separate, preparatory commit):

echo "MSYSTEM=MINGW${{matrix.bitness}}" >>$GITHUB_ENV &&

(The first instance is not inside a matrix, it needs to hard-code MINGW64.)

dscho · 2022-12-06T13:51:54Z

make-file-list.sh

 usr/bin/column.exe
 EOF

+[[ -z "$MINIMAL_GIT" && $MSYSTEM_LOWER == mingw* ]] && cat <<EOF


The [[ ... ]] && syntax is not used anywhere else in this file, therefore it threw me quite a bit. Let's not?

Also, maybe it's better to introduce a new variable like PDFTOTEXT_FILES, and set it in an architecture-specific way before using it here.

I used it so that I could do mingw* (wildcard). test can't do that. Do you have a better alternative to this: if test -z "$MINIMAL_GIT" && (test "$MSYSTEM_LOWER" = "mingw64" || test "$MSYSTEM_LOWER" = "mingw32")

I'm not sure whether I get the comment regarding pdftotext. That executable is supported on all platforms, so no need to introduce architecture-specific logic for it, right?

Right, the wildcard. The classical Unix method is to use case, like so:

case "$MINIMAL_GIT,$MSYSTEM_LOWER" in ,mingw*) ... ;; esac

But that's a lot more verbose, so I guess I'm fine with the [[ construct.

I'm not sure whether I get the comment regarding pdftotext.

What I mean is this: https://github.com/git-for-windows/build-extra/pull/437/files#diff-9b50f413340b32f6bc699741e8b09ab52b3e9c7698d787cc26c4c95e78580916L375-R401

-mingw$BITNESS/bin/pdftotext.exe -mingw$BITNESS/bin/libstdc++-6.dll +$MSYSTEM_LOWER/bin/pdftotext.exe usr/bin/column.exe EOF +if test -z "$MINIMAL_GIT" && (test "$MSYSTEM_LOWER" = "mingw64" || test "$MSYSTEM_LOWER" = "mingw32"); then +cat <<EOF +$MSYSTEM_LOWER/bin/libstdc++-6.dll +EOF +fi +

Separating the pdftotext.exe line from the libstdc++-6.dll line in the proposed way won't make it as obvious as the current version that pdftotext.exe is the only reason why that .dll is required in the first place.

What I would prefer is to have conditionals that add the appropriate lines for pdftotext depending on the platform, unless we're enumerating the files for a MinGit.

So I would define these lines somewhat like this:

case $MSYSTEM_LOWER in mingw*) PDFTOTEXT_FILES="$MSYSTEM_LOWER/bin/pdftotext.exe $MSYSTEM_LOWER/bin/libstdc++-6.dll" ;; *) # In the clang version, we do not need the libstdc++ DLL PDFTOTEXT_FILES="$MSYSTEM_LOWER/bin/pdftotext.exe ;; esac

and then replace the current mentions of these files with $PDFTOTEXT_FILES.

Ah, I thought that libstdc++-6.dll was used for other things as well. Have just updated the commit and addressed the other feedback from this PR as well. Thanks!

dscho · 2022-12-06T13:52:43Z

.github/workflows/main.yml

@@ -4,6 +4,7 @@ on: pull_request

 env:
  GIT_CONFIG_PARAMETERS: "'checkout.workers=56'"
+  MSYSTEM: MINGW64


Since we're not setting MSYSTEM=MINGW32 explicitly in the 32-bit jobs, that's what causes this problem.

Several scripts in build-extra will rely on the MSYSTEM to detect things like the architecture (x86_64, i686, aarch64). This is required because the old solution (uname -m) doesn't work for CLANGARM64, which depends on x64 emulation in MSYS2. Ref: https://www.msys2.org/wiki/arm64/ Signed-off-by: Dennis Ameling <[email protected]>

When using clang-based msystems like CLANG64, the folder prefix is different than what make-file-list.sh is used to (/clangxxx instead of /mingwxx). Let's teach make-file-list.sh to find and filter those files. Signed-off-by: Dennis Ameling <[email protected]>

Adds support for the CLANGARM64 MSYSTEM to make-file-list.sh. Signed-off-by: Dennis Ameling <[email protected]>

clangarm64 is a relatively new MSYSTEM added by the MSYS2 team. Let's leverage it on our journey to Git for Windows ARM64 support. Signed-off-by: Dennis Ameling <[email protected]>

$(uname -m) returns the architecture of the MSYS2 shell. This works for x64 and x86 on Windows, but not for ARM64, for which we'll add support in a follow-up PR. The reason is that MSYS2 can only be run using x64 emulation on ARM64 due to lacking Cygwin support for that platform. To prepare for ARM64 support and to be more consistent with other MSYS2 logic, let's leverage $MSYSTEM instead of $(uname -m). Ref: https://www.msys2.org/wiki/arm64/ Signed-off-by: Dennis Ameling <[email protected]>

This replaces our earlier efforts to support ARM64 for Git for Windows. It uses the new CLANGARM64 MSYSTEM, so ARM64 now becomes a first-class citizen. Signed-off-by: Dennis Ameling <[email protected]>

$(uname -m) returns the architecture of the MSYS2 shell. This works for x64 and x86 on Windows, but not for ARM64, for which we'll add support in a follow-up PR. The reason is that MSYS2 can only be run using x64 emulation on ARM64 due to lacking Cygwin support for that platform. To prepare for ARM64 support and to be more consistent with other MSYS2 logic, let's leverage $MSYSTEM instead of $(uname -m). Ref: https://www.msys2.org/wiki/arm64/ Signed-off-by: Dennis Ameling <[email protected]>

This replaces our earlier efforts to support ARM64 for Git for Windows. It uses the new CLANGARM64 MSYSTEM, so ARM64 now becomes a first-class citizen. Signed-off-by: Dennis Ameling <[email protected]>

Signed-off-by: Dennis Ameling <[email protected]>

dscho

Very nice, thank you so much!

dennisameling mentioned this pull request Sep 25, 2022

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

Closed

dennisameling mentioned this pull request Oct 29, 2022

Enable more clangarm64 packages (connect, antiword, odt2txt) msys2/MINGW-packages#13787

Merged

dennisameling force-pushed the portable-clangarm64 branch 2 times, most recently from 13f6d65 to 4e06cec Compare October 30, 2022 18:59

dennisameling marked this pull request as ready for review October 30, 2022 19:05

dennisameling changed the title ~~[WIP] portable: add CLANGARM64 support~~ portable: add CLANGARM64 support Oct 30, 2022

dennisameling force-pushed the portable-clangarm64 branch from 4e06cec to 157aae6 Compare October 30, 2022 21:04

dennisameling changed the title ~~portable: add CLANGARM64 support~~ portable + installer: add CLANGARM64 support Oct 30, 2022

dennisameling commented Oct 30, 2022

View reviewed changes

dennisameling force-pushed the portable-clangarm64 branch from 157aae6 to 98ad73e Compare October 30, 2022 21:39

dennisameling force-pushed the portable-clangarm64 branch from 98ad73e to 93f19e3 Compare November 30, 2022 00:57

dennisameling commented Nov 30, 2022

View reviewed changes

dennisameling force-pushed the portable-clangarm64 branch 3 times, most recently from 917147d to 6b4653c Compare December 6, 2022 00:41

dennisameling requested a review from dscho December 6, 2022 00:58

rimrul reviewed Dec 6, 2022

View reviewed changes

installer/install.iss Outdated Show resolved Hide resolved

dennisameling force-pushed the portable-clangarm64 branch from 6b4653c to 99c6913 Compare December 6, 2022 11:20

dscho reviewed Dec 6, 2022

View reviewed changes

dennisameling force-pushed the portable-clangarm64 branch 2 times, most recently from c70f4c1 to 110dba5 Compare December 17, 2022 20:06

dennisameling added 5 commits December 17, 2022 21:31

make-file-list.sh: add support for ARM64

2785516

Adds support for the CLANGARM64 MSYSTEM to make-file-list.sh. Signed-off-by: Dennis Ameling <[email protected]>

git-extra.install: add clangarm64

d551cf0

clangarm64 is a relatively new MSYSTEM added by the MSYS2 team. Let's leverage it on our journey to Git for Windows ARM64 support. Signed-off-by: Dennis Ameling <[email protected]>

portable: add CLANGARM64 support

2943e60

This replaces our earlier efforts to support ARM64 for Git for Windows. It uses the new CLANGARM64 MSYSTEM, so ARM64 now becomes a first-class citizen. Signed-off-by: Dennis Ameling <[email protected]>

dennisameling added 2 commits December 17, 2022 21:31

installer: add CLANGARM64 support

3364ef0

This replaces our earlier efforts to support ARM64 for Git for Windows. It uses the new CLANGARM64 MSYSTEM, so ARM64 now becomes a first-class citizen. Signed-off-by: Dennis Ameling <[email protected]>

dennisameling force-pushed the portable-clangarm64 branch from 110dba5 to f90fe17 Compare December 17, 2022 20:31

git-extra: bump version

2fa6f5c

Signed-off-by: Dennis Ameling <[email protected]>

dennisameling force-pushed the portable-clangarm64 branch from f90fe17 to 2fa6f5c Compare December 17, 2022 20:34

dscho approved these changes Dec 17, 2022

View reviewed changes

dscho merged commit f2bb9e5 into git-for-windows:main Dec 17, 2022

dennisameling deleted the portable-clangarm64 branch December 17, 2022 21:04

portable + installer: add CLANGARM64 support #437

portable + installer: add CLANGARM64 support #437

Uh oh!

Conversation

dennisameling commented Sep 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

How to test

Uh oh!

rimrul commented Sep 26, 2022

Uh oh!

dennisameling commented Oct 29, 2022

Uh oh!

dennisameling left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dennisameling Dec 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dennisameling Nov 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dennisameling commented Dec 6, 2022

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dscho left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dennisameling commented Sep 24, 2022 •

edited

Loading

dennisameling Dec 6, 2022 •

edited

Loading

dennisameling Nov 30, 2022 •

edited

Loading