Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Start support for MSYSTEM=ARM64 #321

Merged
merged 1 commit into from
Jan 27, 2021
Merged

Start support for MSYSTEM=ARM64 #321

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions git-extra/git-extra.install.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -230,6 +230,32 @@ test -d "$TMPDIR" || test ! -d "$TMP" || {\
#
# As we no longer need it, delete it.
test ! -h /ssl || rm /ssl

# Allow an i686 version of Git for Windows, augmented by `/arm64/*`, to serve
# as sort of an ARM64 version of Git for Windows (because Windows/ARM64 can run
# i686 executables via a built-in emulator).
test i686 != "$arch" ||
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure about this line. Using the 64-bit SDK on another machine and then it doesn't work. When I remove this line, /etc/profile is updated correctly in the SDK root folder 🚀 Benefit of removing this line is that this patch will be applied both to git-sdk-32 and git-sdk-64, so then it doesn't matter which SDK folks use to build Git. This shouldn't hurt anyone unless their MSYSTEM is set to ARM64 😊

Will do some more tests later today, but at least for now /etc/profile is updated correctly 🎉

image

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure about this line. Using the 64-bit SDK on another machine and then it doesn't work.

That's on purpose, actually: you will not find the case statement in the 64-bit SDK. Instead, it was moved to /etc/msystem there.

The reason why the 32-bit SDK has a different /etc/profile is that its MSYS part hasn't been updated in a long time. In other words, it does not reflect the newest filesystems package definition.

When I remove this line, /etc/profile is updated correctly in the SDK root folder 🚀 Benefit of removing this line is that this patch will be applied both to git-sdk-32 and git-sdk-64, so then it doesn't matter which SDK folks use to build Git.

For the moment, I would rather leave out the complication of dealing with both git-sdk-32 and git-sdk-64, but focus on getting it working correctly only in the git-sdk-32 case.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for now /etc/profile is updated correctly

🎉

My one concern is that the MANPATH is now set exclusively to /arm64/{local,share}/man, but there is nothing there...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So if I understand it correctly, MANPATH is for the Git for Windows manual files (https://git-scm.com/docs/git#Documentation/git.txt---man-path)? When is this variable actually being used?

Copy link
Contributor

@dennisameling dennisameling Jan 22, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I created a portable installer and it correctly includes the updated /etc/profile 🚀

What would be the correct order of steps to build a portable installer with the arm64 binaries? Something like this?

  • sdk build mingw-w64-git
  • pacman -U mingw-w64-i686-git-2.30.0.2.f8cbc844b8-1-any.pkg.tar.xz
  • sdk build git-extra (so that /etc/profile gets overwritten to include ARM64)
  • pacman -U git-extra-1.1.513.20ab9a5-1-i686.pkg.tar.xz
  • (create the arm64 binaries now and output to C:\git-sdk-32\arm64)
  • Build the portable installer with the --cross-compile-arm64 option, see portable: add arm64 artifact option #323 for details

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So if I understand it correctly, MANPATH is for the Git for Windows manual files (https://git-scm.com/docs/git#Documentation/git.txt---man-path)? When is this variable actually being used?

When you call something like man bash in the SDK. This issue is not that important given that we ship Git for Windows without man.exe, but still...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shall we leave it as-is for now then?

Just to confirm, this is the updated section in /etc/profile after running sdk build git-extra:

MINGW32)
  MINGW_MOUNT_POINT="${MINGW_PREFIX}"
  PATH="${MINGW_MOUNT_POINT}/bin:${MSYS2_PATH}${ORIGINAL_PATH:+:${ORIGINAL_PATH}}"
  PKG_CONFIG_PATH="${MINGW_MOUNT_POINT}/lib/pkgconfig:${MINGW_MOUNT_POINT}/share/pkgconfig"
  ACLOCAL_PATH="${MINGW_MOUNT_POINT}/share/aclocal:/usr/share/aclocal"
  MANPATH="${MINGW_MOUNT_POINT}/local/man:${MINGW_MOUNT_POINT}/share/man:${MANPATH}"
  ;;
MINGW64)
  MINGW_MOUNT_POINT="${MINGW_PREFIX}"
  PATH="${MINGW_MOUNT_POINT}/bin:${MSYS2_PATH}${ORIGINAL_PATH:+:${ORIGINAL_PATH}}"
  PKG_CONFIG_PATH="${MINGW_MOUNT_POINT}/lib/pkgconfig:${MINGW_MOUNT_POINT}/share/pkgconfig"
  ACLOCAL_PATH="${MINGW_MOUNT_POINT}/share/aclocal:/usr/share/aclocal"
  MANPATH="${MINGW_MOUNT_POINT}/local/man:${MINGW_MOUNT_POINT}/share/man:${MANPATH}"
  ;;
ARM64)
  MINGW_MOUNT_POINT="/arm64"
  PATH="${MINGW_MOUNT_POINT}/bin:/mingw32/bin:${MSYS2_PATH}${ORIGINAL_PATH:+:${ORIGINAL_PATH}}"
  PKG_CONFIG_PATH="${MINGW_MOUNT_POINT}/lib/pkgconfig:${MINGW_MOUNT_POINT}/share/pkgconfig"
  ACLOCAL_PATH="${MINGW_MOUNT_POINT}/share/aclocal:/usr/share/aclocal"
  MANPATH="${MINGW_MOUNT_POINT}/local/man:${MINGW_MOUNT_POINT}/share/man:${MANPATH}"
  ;;

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't happy leaving it as-is, in particular the rather non-intuitive sed invocation. @dennisameling could you please test again, and also sanity-check the new revision (it should be eminently more readable than before)?

Copy link
Contributor

@dennisameling dennisameling Jan 26, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dscho just tested again, the results:

✔️ Running sdk build git-extra and then pacman -U git-extra-...-tar.xz correctly updates /etc/profile in git-sdk-32:

ARM64)
  MINGW_MOUNT_POINT="/arm64"
  PATH="${MINGW_MOUNT_POINT}/bin:/mingw32/bin:${MSYS2_PATH}${ORIGINAL_PATH:+:${ORIGINAL_PATH}}"
  PKG_CONFIG_PATH="${MINGW_MOUNT_POINT}/lib/pkgconfig:${MINGW_MOUNT_POINT}/share/pkgconfig:/mingw32/lib/pkgconfig:/mingw32/share/pkgconfig"
  ACLOCAL_PATH="${MINGW_MOUNT_POINT}/share/aclocal:/mingw32/share/aclocal:/usr/share/aclocal"
  MANPATH="${MINGW_MOUNT_POINT}/local/man:${MINGW_MOUNT_POINT}/share/man:/mingw32/local/man:/mingw32/share/man:${MANPATH}"
  ;;

✔️ Things are looking good in ARM64 Git:

image

Thanks! And yes, the code is much easier to understand now, thank you 😅

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perfect. Time to merge, then.

grep -q '^ARM64)$' /etc/profile ||
sed -i '/^MINGW64)/{
# read entire `case` arm until `;;`
:1;N;/[^;]$/b1;

# print out the MINGW64 `case` arm unchanged
p;

# now, transmogrify it into the ARM64 `case` arm:
s|^MINGW|ARM|;
# change the MINGW_PREFIX
s|\${MINGW_PREFIX}|/arm64|;
# add the /mingw32/bin fall-back to the PATH
s| PATH=[^:]*|&:/mingw32/bin|;
# adjust the pkgconfig path
s|/share/pkgconfig|&:/mingw32/lib/pkgconfig:/mingw32/share/pkgconfig|;
# adjust autoconf path
s|:/usr/share/aclocal|:/mingw32/share/aclocal&|;
# add the /mingw32 fall-back paths to MANPATH
s|:\${MANPATH}|:/mingw32/local/man:/mingw32/share/man&|
}' /etc/profile
}

post_upgrade () {
Expand Down