clangarm64: let the tests pass! #5586
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dscho
force-pushed
the
t0060-and-t6700-vs-clangarm64
branch
from
f55ea3d to
9f8440d
Compare
Merged
dscho
added a commit
that referenced
this pull request
Apr 16, 2025
I noticed that the CI builds of `shears/main` were failing, as well as the PR builds of #5586. The reasons are outside of Git for Windows, but the fixes need to be inside anyway.
git-for-windows-ci
pushed a commit
that referenced
this pull request
Apr 16, 2025
I noticed that the CI builds of `shears/main` were failing, as well as the PR builds of #5586. The reasons are outside of Git for Windows, but the fixes need to be inside anyway.
git-for-windows-ci
pushed a commit
that referenced
this pull request
Apr 16, 2025
I noticed that the CI builds of `shears/main` were failing, as well as the PR builds of #5586. The reasons are outside of Git for Windows, but the fixes need to be inside anyway.
rimrul
reviewed
Apr 17, 2025
rimrul
reviewed
Apr 17, 2025
The original test case obviously only worked in the x86_64 flavor of Git for Windows (and since I rarely test on i686, this was not caught). This completely breaks on Windows/ARM64: +++ GIT_DIR=/dev/null +++ git diff --no-index --ignore-cr-at-eol -- expect actual diff --git a/expect b/actual index e0dc09e..2301bbf 100644 --- a/expect +++ b/actual @@ -1,3 +1,3 @@ -MSYSTEM=CLANGARM64 +MSYSTEM=MINGW64 mingw64 usr error: last command exited with $?=1 not ok 221 - MSYSTEM/PATH is adjusted if necessary To fix that, let's rely on `MINGW_PREFIX` (falling back to constructing it from `MSYSTEM`, defaulting to `MINGW64` if _that_ is unset, too). Signed-off-by: Johannes Schindelin <[email protected]>
Let's make the `.dll` copying a bit more robust. At least in my hands, the check sometimes failed to detect whether there are any `.dll` files to be copied. Signed-off-by: Johannes Schindelin <[email protected]>
When this commit was originally introduced, Windows/ARM64 support was a fantasy. In the meantime it is reality and we need to do a better job of setting `MSYSTEM` and the `PATH`: We want to support Windows/ARM64 properly. Signed-off-by: Johannes Schindelin <[email protected]>
dscho
force-pushed
the
t0060-and-t6700-vs-clangarm64
branch
from
9f8440d to
bac70b1
Compare
Just as in b64d78a (max_tree_depth: lower it for MSVC to avoid stack overflows, 2023-11-01), I encountered the same problem with the clang builds on Windows/ARM64. The symptom is an exit code 127 when t6700 tries to verify that `git archive big` fails. This exit code is reserved on Unix/Linux to mean "command not found". Unfortunately in this case, it is the fall-back chosen by Cygwin's `pinfo::status_exit()` method when encountering the NSTATUS `STATUS_STACK_OVERFLOW`, see https://github.com/cygwin/cygwin/blob/cygwin-3.6.1/winsup/cygwin/pinfo.cc#L171 I verified manually that the stack overflow always happens somewhere around tree depth 1403, therefore 1280 should be a safe bound in these instances. Signed-off-by: Johannes Schindelin <[email protected]>
dscho
force-pushed
the
t0060-and-t6700-vs-clangarm64
branch
from
bac70b1 to
2c3b779
Compare
|
Here is a CI run that verifies that this branch (plus a few patches for debugging) builds and passes the test suite. |
rimrul
approved these changes
Apr 21, 2025
dscho
added a commit
that referenced
this pull request
Apr 25, 2025
I encountered these issues that had hitherto escaped us [when I worked on letting the `ci-artifacts` workflow in git-sdk-arm64 also build Git and run the test suite](git-for-windows/git-sdk-arm64#37) by way of validating the `minimal-sdk` artifact. Mind, this PR does not only adjust a test case that was previously too fixated on x86_64. There are two real issues that this PR addresses and that were found via the test suite: - When the environment variable `MSYSTEM` is not yet set, it now is set appropriately even on Windows/ARM64 (and the `PATH` is adjusted accordingly). - The tree traversal limit designed to avoid stack overflows needed to be adjusted for the clangarm64 builds.
dscho
added a commit
that referenced
this pull request
Apr 25, 2025
I encountered these issues that had hitherto escaped us [when I worked on letting the `ci-artifacts` workflow in git-sdk-arm64 also build Git and run the test suite](git-for-windows/git-sdk-arm64#37) by way of validating the `minimal-sdk` artifact. Mind, this PR does not only adjust a test case that was previously too fixated on x86_64. There are two real issues that this PR addresses and that were found via the test suite: - When the environment variable `MSYSTEM` is not yet set, it now is set appropriately even on Windows/ARM64 (and the `PATH` is adjusted accordingly). - The tree traversal limit designed to avoid stack overflows needed to be adjusted for the clangarm64 builds.
dscho
added a commit
that referenced
this pull request
Apr 25, 2025
I encountered these issues that had hitherto escaped us [when I worked on letting the `ci-artifacts` workflow in git-sdk-arm64 also build Git and run the test suite](git-for-windows/git-sdk-arm64#37) by way of validating the `minimal-sdk` artifact. Mind, this PR does not only adjust a test case that was previously too fixated on x86_64. There are two real issues that this PR addresses and that were found via the test suite: - When the environment variable `MSYSTEM` is not yet set, it now is set appropriately even on Windows/ARM64 (and the `PATH` is adjusted accordingly). - The tree traversal limit designed to avoid stack overflows needed to be adjusted for the clangarm64 builds.
dscho
added a commit
that referenced
this pull request
Apr 30, 2025
I encountered these issues that had hitherto escaped us [when I worked on letting the `ci-artifacts` workflow in git-sdk-arm64 also build Git and run the test suite](git-for-windows/git-sdk-arm64#37) by way of validating the `minimal-sdk` artifact. Mind, this PR does not only adjust a test case that was previously too fixated on x86_64. There are two real issues that this PR addresses and that were found via the test suite: - When the environment variable `MSYSTEM` is not yet set, it now is set appropriately even on Windows/ARM64 (and the `PATH` is adjusted accordingly). - The tree traversal limit designed to avoid stack overflows needed to be adjusted for the clangarm64 builds.
dscho
added a commit
that referenced
this pull request
Apr 30, 2025
I encountered these issues that had hitherto escaped us [when I worked on letting the `ci-artifacts` workflow in git-sdk-arm64 also build Git and run the test suite](git-for-windows/git-sdk-arm64#37) by way of validating the `minimal-sdk` artifact. Mind, this PR does not only adjust a test case that was previously too fixated on x86_64. There are two real issues that this PR addresses and that were found via the test suite: - When the environment variable `MSYSTEM` is not yet set, it now is set appropriately even on Windows/ARM64 (and the `PATH` is adjusted accordingly). - The tree traversal limit designed to avoid stack overflows needed to be adjusted for the clangarm64 builds.
dscho
added a commit
to dscho/git
that referenced
this pull request
Apr 30, 2025
This patch makes it so that Git really sets the desired `MSYSTEM` on Windows/ARM64. The reason this did not work (and my faulty tests did not catch that before merging git-for-windows#5586) is that clang (at least version 20.1.3 as built from https://github.com/git-for-windows/MINGW-packages 8df0c2fff4184deff15acce9bfd791fb6e0d60fe) predefines the `__MINGW64__` constant: $ echo | clang -dM -E - | grep -n MINGW64 249:#define __MINGW64__ 1 Let's just switch the order between the CLANGARM64 and the MINGW64 test; This will still work for MINGW64 because none of the constants used in the CLANGARM64 condition are defined for x64 gcc (or for that matter, clang). Signed-off-by: Johannes Schindelin <[email protected]>
dscho
added a commit
that referenced
this pull request
Apr 30, 2025
Apparently my tests of #5586 had been incomplete. This here patch is needed to let t0060.221 pass (where it verifies that `git.exe` sets `MSYSTEM` correctly if that environment variable has not yet been set): Simply reverse the order of the tests whether to set `MINGW64` or `CLANGARM64` to avoid using the former on Windows/ARM64 by mistake.
dscho
added a commit
that referenced
this pull request
Apr 30, 2025
I encountered these issues that had hitherto escaped us [when I worked on letting the `ci-artifacts` workflow in git-sdk-arm64 also build Git and run the test suite](git-for-windows/git-sdk-arm64#37) by way of validating the `minimal-sdk` artifact. Mind, this PR does not only adjust a test case that was previously too fixated on x86_64. There are two real issues that this PR addresses and that were found via the test suite: - When the environment variable `MSYSTEM` is not yet set, it now is set appropriately even on Windows/ARM64 (and the `PATH` is adjusted accordingly). - The tree traversal limit designed to avoid stack overflows needed to be adjusted for the clangarm64 builds.
dscho
added a commit
that referenced
this pull request
Apr 30, 2025
I encountered these issues that had hitherto escaped us [when I worked on letting the `ci-artifacts` workflow in git-sdk-arm64 also build Git and run the test suite](git-for-windows/git-sdk-arm64#37) by way of validating the `minimal-sdk` artifact. Mind, this PR does not only adjust a test case that was previously too fixated on x86_64. There are two real issues that this PR addresses and that were found via the test suite: - When the environment variable `MSYSTEM` is not yet set, it now is set appropriately even on Windows/ARM64 (and the `PATH` is adjusted accordingly). - The tree traversal limit designed to avoid stack overflows needed to be adjusted for the clangarm64 builds.
dscho
added a commit
that referenced
this pull request
Apr 30, 2025
I encountered these issues that had hitherto escaped us [when I worked on letting the `ci-artifacts` workflow in git-sdk-arm64 also build Git and run the test suite](git-for-windows/git-sdk-arm64#37) by way of validating the `minimal-sdk` artifact. Mind, this PR does not only adjust a test case that was previously too fixated on x86_64. There are two real issues that this PR addresses and that were found via the test suite: - When the environment variable `MSYSTEM` is not yet set, it now is set appropriately even on Windows/ARM64 (and the `PATH` is adjusted accordingly). - The tree traversal limit designed to avoid stack overflows needed to be adjusted for the clangarm64 builds.
dscho
added a commit
that referenced
this pull request
Apr 30, 2025
This patch makes it so that Git really sets the desired `MSYSTEM` on Windows/ARM64. The reason this did not work (and my faulty tests did not catch that before merging #5586) is that clang (at least version 20.1.3 as built from https://github.com/git-for-windows/MINGW-packages 8df0c2fff4184deff15acce9bfd791fb6e0d60fe) predefines the `__MINGW64__` constant: $ echo | clang -dM -E - | grep -n MINGW64 249:#define __MINGW64__ 1 Let's just switch the order between the CLANGARM64 and the MINGW64 test; This will still work for MINGW64 because none of the constants used in the CLANGARM64 condition are defined for x64 gcc (or for that matter, clang). Signed-off-by: Johannes Schindelin <[email protected]>
dscho
added a commit
that referenced
this pull request
Apr 30, 2025
I encountered these issues that had hitherto escaped us [when I worked on letting the `ci-artifacts` workflow in git-sdk-arm64 also build Git and run the test suite](git-for-windows/git-sdk-arm64#37) by way of validating the `minimal-sdk` artifact. Mind, this PR does not only adjust a test case that was previously too fixated on x86_64. There are two real issues that this PR addresses and that were found via the test suite: - When the environment variable `MSYSTEM` is not yet set, it now is set appropriately even on Windows/ARM64 (and the `PATH` is adjusted accordingly). - The tree traversal limit designed to avoid stack overflows needed to be adjusted for the clangarm64 builds.
dscho
added a commit
that referenced
this pull request
May 1, 2025
I encountered these issues that had hitherto escaped us [when I worked on letting the `ci-artifacts` workflow in git-sdk-arm64 also build Git and run the test suite](git-for-windows/git-sdk-arm64#37) by way of validating the `minimal-sdk` artifact. Mind, this PR does not only adjust a test case that was previously too fixated on x86_64. There are two real issues that this PR addresses and that were found via the test suite: - When the environment variable `MSYSTEM` is not yet set, it now is set appropriately even on Windows/ARM64 (and the `PATH` is adjusted accordingly). - The tree traversal limit designed to avoid stack overflows needed to be adjusted for the clangarm64 builds.
dscho
added a commit
that referenced
this pull request
May 1, 2025
I encountered these issues that had hitherto escaped us [when I worked on letting the `ci-artifacts` workflow in git-sdk-arm64 also build Git and run the test suite](git-for-windows/git-sdk-arm64#37) by way of validating the `minimal-sdk` artifact. Mind, this PR does not only adjust a test case that was previously too fixated on x86_64. There are two real issues that this PR addresses and that were found via the test suite: - When the environment variable `MSYSTEM` is not yet set, it now is set appropriately even on Windows/ARM64 (and the `PATH` is adjusted accordingly). - The tree traversal limit designed to avoid stack overflows needed to be adjusted for the clangarm64 builds.
dscho
added a commit
that referenced
this pull request
May 1, 2025
This patch makes it so that Git really sets the desired `MSYSTEM` on Windows/ARM64. The reason this did not work (and my faulty tests did not catch that before merging #5586) is that clang (at least version 20.1.3 as built from https://github.com/git-for-windows/MINGW-packages 8df0c2fff4184deff15acce9bfd791fb6e0d60fe) predefines the `__MINGW64__` constant: $ echo | clang -dM -E - | grep -n MINGW64 249:#define __MINGW64__ 1 Let's just switch the order between the CLANGARM64 and the MINGW64 test; This will still work for MINGW64 because none of the constants used in the CLANGARM64 condition are defined for x64 gcc (or for that matter, clang). Signed-off-by: Johannes Schindelin <[email protected]>
git-for-windows-ci
pushed a commit
that referenced
this pull request
May 1, 2025
I encountered these issues that had hitherto escaped us [when I worked on letting the `ci-artifacts` workflow in git-sdk-arm64 also build Git and run the test suite](git-for-windows/git-sdk-arm64#37) by way of validating the `minimal-sdk` artifact. Mind, this PR does not only adjust a test case that was previously too fixated on x86_64. There are two real issues that this PR addresses and that were found via the test suite: - When the environment variable `MSYSTEM` is not yet set, it now is set appropriately even on Windows/ARM64 (and the `PATH` is adjusted accordingly). - The tree traversal limit designed to avoid stack overflows needed to be adjusted for the clangarm64 builds.
git-for-windows-ci
pushed a commit
that referenced
this pull request
May 1, 2025
I encountered these issues that had hitherto escaped us [when I worked on letting the `ci-artifacts` workflow in git-sdk-arm64 also build Git and run the test suite](git-for-windows/git-sdk-arm64#37) by way of validating the `minimal-sdk` artifact. Mind, this PR does not only adjust a test case that was previously too fixated on x86_64. There are two real issues that this PR addresses and that were found via the test suite: - When the environment variable `MSYSTEM` is not yet set, it now is set appropriately even on Windows/ARM64 (and the `PATH` is adjusted accordingly). - The tree traversal limit designed to avoid stack overflows needed to be adjusted for the clangarm64 builds.
git-for-windows-ci
pushed a commit
that referenced
this pull request
May 1, 2025
This patch makes it so that Git really sets the desired `MSYSTEM` on Windows/ARM64. The reason this did not work (and my faulty tests did not catch that before merging #5586) is that clang (at least version 20.1.3 as built from https://github.com/git-for-windows/MINGW-packages 8df0c2fff4184deff15acce9bfd791fb6e0d60fe) predefines the `__MINGW64__` constant: $ echo | clang -dM -E - | grep -n MINGW64 249:#define __MINGW64__ 1 Let's just switch the order between the CLANGARM64 and the MINGW64 test; This will still work for MINGW64 because none of the constants used in the CLANGARM64 condition are defined for x64 gcc (or for that matter, clang). Signed-off-by: Johannes Schindelin <[email protected]>
git-for-windows-ci
pushed a commit
that referenced
this pull request
May 1, 2025
I encountered these issues that had hitherto escaped us [when I worked on letting the `ci-artifacts` workflow in git-sdk-arm64 also build Git and run the test suite](git-for-windows/git-sdk-arm64#37) by way of validating the `minimal-sdk` artifact. Mind, this PR does not only adjust a test case that was previously too fixated on x86_64. There are two real issues that this PR addresses and that were found via the test suite: - When the environment variable `MSYSTEM` is not yet set, it now is set appropriately even on Windows/ARM64 (and the `PATH` is adjusted accordingly). - The tree traversal limit designed to avoid stack overflows needed to be adjusted for the clangarm64 builds.
git-for-windows-ci
pushed a commit
that referenced
this pull request
May 3, 2025
I encountered these issues that had hitherto escaped us [when I worked on letting the `ci-artifacts` workflow in git-sdk-arm64 also build Git and run the test suite](git-for-windows/git-sdk-arm64#37) by way of validating the `minimal-sdk` artifact. Mind, this PR does not only adjust a test case that was previously too fixated on x86_64. There are two real issues that this PR addresses and that were found via the test suite: - When the environment variable `MSYSTEM` is not yet set, it now is set appropriately even on Windows/ARM64 (and the `PATH` is adjusted accordingly). - The tree traversal limit designed to avoid stack overflows needed to be adjusted for the clangarm64 builds.
git-for-windows-ci
pushed a commit
that referenced
this pull request
May 6, 2025
I encountered these issues that had hitherto escaped us [when I worked on letting the `ci-artifacts` workflow in git-sdk-arm64 also build Git and run the test suite](git-for-windows/git-sdk-arm64#37) by way of validating the `minimal-sdk` artifact. Mind, this PR does not only adjust a test case that was previously too fixated on x86_64. There are two real issues that this PR addresses and that were found via the test suite: - When the environment variable `MSYSTEM` is not yet set, it now is set appropriately even on Windows/ARM64 (and the `PATH` is adjusted accordingly). - The tree traversal limit designed to avoid stack overflows needed to be adjusted for the clangarm64 builds.
git-for-windows-ci
pushed a commit
that referenced
this pull request
May 6, 2025
I encountered these issues that had hitherto escaped us [when I worked on letting the `ci-artifacts` workflow in git-sdk-arm64 also build Git and run the test suite](git-for-windows/git-sdk-arm64#37) by way of validating the `minimal-sdk` artifact. Mind, this PR does not only adjust a test case that was previously too fixated on x86_64. There are two real issues that this PR addresses and that were found via the test suite: - When the environment variable `MSYSTEM` is not yet set, it now is set appropriately even on Windows/ARM64 (and the `PATH` is adjusted accordingly). - The tree traversal limit designed to avoid stack overflows needed to be adjusted for the clangarm64 builds.
git-for-windows-ci
pushed a commit
that referenced
this pull request
May 8, 2025
I encountered these issues that had hitherto escaped us [when I worked on letting the `ci-artifacts` workflow in git-sdk-arm64 also build Git and run the test suite](git-for-windows/git-sdk-arm64#37) by way of validating the `minimal-sdk` artifact. Mind, this PR does not only adjust a test case that was previously too fixated on x86_64. There are two real issues that this PR addresses and that were found via the test suite: - When the environment variable `MSYSTEM` is not yet set, it now is set appropriately even on Windows/ARM64 (and the `PATH` is adjusted accordingly). - The tree traversal limit designed to avoid stack overflows needed to be adjusted for the clangarm64 builds.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I encountered these issues that had hitherto escaped us when I worked on letting the
ci-artifactsworkflow in git-sdk-arm64 also build Git and run the test suite by way of validating theminimal-sdkartifact.Mind, this PR does not only adjust a test case that was previously too fixated on x86_64. There are two real issues that this PR addresses and that were found via the test suite:
MSYSTEMis not yet set, it now is set appropriately even on Windows/ARM64 (and thePATHis adjusted accordingly).