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.

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

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

Closed
wants to merge 4 commits into from

Conversation

tommyvct
Copy link

No description provided.

etc/profile Outdated Show resolved Hide resolved
etc/profile Outdated
@@ -61,6 +61,13 @@ MINGW64)
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="${MINGW_PREFIX}"
Copy link

@dennisameling dennisameling Dec 28, 2020

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

@@ -61,6 +61,13 @@ MINGW64)
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="${MINGW_PREFIX}"
PATH="${MINGW_MOUNT_POINT}/bin:/mingw32/bin:${MSYS2_PATH}${ORIGINAL_PATH:+:${ORIGINAL_PATH}}"

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

Copy link
Author

@tommyvct tommyvct Dec 28, 2020

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
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).

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
Copy link

dennisameling commented Dec 28, 2020

@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
Author

@dennisameling I'll try.

@dscho
Copy link
Member

dscho commented Jan 5, 2021

@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.

Copy link

@dennisameling dennisameling left a comment

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
Copy link
Member

dscho commented Jan 21, 2021

@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
Author

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

@dennisameling
Copy link

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

@dscho
Copy link
Member

dscho commented Jan 22, 2021

@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

Also works @dscho 😄 👍🏼

@dscho
Copy link
Member

dscho commented Jan 22, 2021

@dennisameling
Copy link

dennisameling commented Feb 4, 2021

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

@dscho
Copy link
Member

dscho commented Feb 4, 2021

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