Skip to content

added MSYSTEM=ARM64 case in /etc/profile#6

Closed
tommyvct wants to merge 4 commits into
git-for-windows:mainfrom
tommyvct:main
Closed

added MSYSTEM=ARM64 case in /etc/profile#6
tommyvct wants to merge 4 commits into
git-for-windows:mainfrom
tommyvct:main

Conversation

@tommyvct

Copy link
Copy Markdown

No description provided.

Comment thread etc/profile Outdated
Comment thread etc/profile Outdated
MANPATH="${MINGW_MOUNT_POINT}/local/man:${MINGW_MOUNT_POINT}/share/man:${MANPATH}"
;;
ARM64)
MINGW_MOUNT_POINT="${MINGW_PREFIX}"

@dennisameling dennisameling Dec 28, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

MINGW_PREFIX is empty on my arm64 test build 🤷🏼‍♂️

I saw some references to MINGW_PREFIX in various places and it mostly relies on uname -m. Since Bash will remain a 32-bit executable for now, it's probably best if we set `MINGW_MOUNT_POINT="/arm64" here.

Before setting MINGW_MOUNT_POINT to /arm64:

image

After:

image

Comment thread etc/profile
;;
ARM64)
MINGW_MOUNT_POINT="${MINGW_PREFIX}"
PATH="${MINGW_MOUNT_POINT}/bin:/mingw32/bin:${MSYS2_PATH}${ORIGINAL_PATH:+:${ORIGINAL_PATH}}"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We don't need mingw32/bin in here anymore if we choose to go down the path from git-for-windows/MINGW-packages#44 and rename mingw32 to arm64 prior to copying the arm64 executables into it:

image

Curious to hear your thoughts on this

@tommyvct tommyvct Dec 28, 2020

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

That's @dscho 's idea.
#6 (comment)
I have no preference on this.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I would strongly recommend not renaming mingw32 to arm64, but instead adding the latter. Then, PATH can be configured in such a way that the binaries from arm64/bin/ are preferred over those from mingw32/bin/, and we're free to only provide a subset of the .exe files for ARM64 (with the rest just being used in their i686 form).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Makes sense. Just applied that in git-for-windows/MINGW-packages#44; arm64 comes first in the path, then mingw32.

@dennisameling

dennisameling commented Dec 28, 2020

Copy link
Copy Markdown
Member

@tommyvct would you mind creating a PR for https://github.com/git-for-windows/git-sdk-64 as well with these changes? So that both SDKs are consistent 🚀

@tommyvct

Copy link
Copy Markdown
Author

@dennisameling I'll try.

@dscho

dscho commented Jan 5, 2021

Copy link
Copy Markdown
Member

@tommyvct would you mind creating a PR for https://github.com/git-for-windows/git-sdk-64 as well with these changes? So that both SDKs are consistent 🚀

Eventually, this change needs to be converted into a part of the post_install scriptlet of git-extra.install.in. This is executed whenever git-extra is installed, and can edit files owned by other packages, which is necessary in case of /etc/profile because it is owned by the filesystem Pacman package of MSYS2. You will see plenty of prior art in the post_install function.

@dennisameling dennisameling left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can confirm this PR works with the native arm64 binaries 🚀 It takes native binaries when available, and otherwise falls back to the /mingw32/bin folder 👍🏼 nice!!

image

@dscho

dscho commented Jan 21, 2021

Copy link
Copy Markdown
Member

@tommyvct would you mind creating a PR for https://github.com/git-for-windows/git-sdk-64 as well with these changes? So that both SDKs are consistent 🚀

Eventually, this change needs to be converted into a part of the post_install scriptlet of git-extra.install.in. This is executed whenever git-extra is installed, and can edit files owned by other packages, which is necessary in case of /etc/profile because it is owned by the filesystem Pacman package of MSYS2. You will see plenty of prior art in the post_install function.

Something like this should work, maybe added directly after this line:

test i686 != "$arch" ||
grep -q '^ARM64)$' /etc/profile ||
sed -i  '/^MINGW64)/{
  :1;N;/[^;]$/b1;
  s|^MINGW\(.*\)\${MINGW_PREFIX}\(.* PATH=[^:]*\)\(.*\)$|&\nARM\1/arm64\2:/mingw32/bin\3|
}' /etc/profile

Note: the change I propose will have to be offered as a PR in https://github.com/git-for-windows/build-extra, and the change can be tested by building the git-extra package via sdk cd git-extra && sdk build and then running pacman -U <package>. After that, /etc/profile should have the expected changes.

@tommyvct

Copy link
Copy Markdown
Author

My school work just started rolling in, I'll try what I can do.

@dennisameling

Copy link
Copy Markdown
Member

@tommyvct just ping me if you need some help 👍🏼 good luck with school!

@dscho

dscho commented Jan 22, 2021

Copy link
Copy Markdown
Member

@tommyvct just ping me if you need some help 👍🏼 good luck with school!

@dennisameling tell you what: I'll do the PR and ask you to verify that it works?

@dennisameling

Copy link
Copy Markdown
Member

Also works @dscho 😄 👍🏼

@dscho

dscho commented Jan 22, 2021

Copy link
Copy Markdown
Member

@dennisameling

dennisameling commented Feb 4, 2021

Copy link
Copy Markdown
Member

This PR might now be closed since this change was added automatically by 977f2bc

@dscho

dscho commented Feb 4, 2021

Copy link
Copy Markdown
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants