From f591a9bfebce123acca44ede12a4327ec1804b65 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Thu, 18 Jan 2024 11:22:45 +0100 Subject: [PATCH 1/5] t7527: decrease likelihood of racing with fsmonitor daemon In t7527, we test that the builtin fsmonitor daemon works well in various edge cases. One of these tests is frequently failing because events reported by the fsmonitor--daemon are missing an expected event. This failure is essentially a race condition: we do not wait for the daemon to flush out all events before we ask it to quit. Consequently, it can happen that we miss some expected events. In other testcases we counteract this race by sending a simple query to the daemon. Quoting a comment: We run a simple query after modifying the filesystem just to introduce a bit of a delay so that the trace logging from the daemon has time to get flushed to disk. Now this workaround is not a "proper" fix as we do not wait for all events to have been synchronized in a deterministic way. But this fix seems to be sufficient for all the other tests to pass, so it must not be all that bad. Convert the failing test to do the same. While the test was previously failing in about 50% of the test runs, I couldn't reproduce the failure after the change anymore. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- t/t7527-builtin-fsmonitor.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/t/t7527-builtin-fsmonitor.sh b/t/t7527-builtin-fsmonitor.sh index 78503158fd..363f9dc0e4 100755 --- a/t/t7527-builtin-fsmonitor.sh +++ b/t/t7527-builtin-fsmonitor.sh @@ -978,7 +978,7 @@ test_expect_success !UNICODE_COMPOSITION_SENSITIVE 'Unicode nfc/nfd' ' mkdir test_unicode/nfd && mkdir test_unicode/nfd/d_${utf8_nfd} && - git -C test_unicode fsmonitor--daemon stop && + test-tool -C test_unicode fsmonitor-client query --token 0 && if test_have_prereq UNICODE_NFC_PRESERVED then From d52b426ad4b038e09421c2e3a3c991832e1eec97 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Thu, 18 Jan 2024 11:22:49 +0100 Subject: [PATCH 2/5] Makefile: detect new Homebrew location for ARM-based Macs With the introduction of the ARM-based Macs the default location for Homebrew has changed from "/usr/local" to "/opt/homebrew". We only handle the former location though, which means that unless the user has manually configured required search paths we won't be able to locate it. Improve upon this by adding relevant paths to our CFLAGS and LDFLAGS as well as detecting the location of msgfmt(1). Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- config.mak.uname | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/config.mak.uname b/config.mak.uname index 3bb03f423a..dacc95172d 100644 --- a/config.mak.uname +++ b/config.mak.uname @@ -158,6 +158,19 @@ ifeq ($(uname_S),Darwin) ifeq ($(shell test -x /usr/local/opt/gettext/bin/msgfmt && echo y),y) MSGFMT = /usr/local/opt/gettext/bin/msgfmt endif + # On newer ARM-based machines the default installation path has changed to + # /opt/homebrew. Include it in our search paths so that the user does not + # have to configure this manually. + # + # Note that we do not employ the same workaround as above where we manually + # add gettext. The issue was fixed more than three years ago by now, and at + # that point there haven't been any ARM-based Macs yet. + else ifeq ($(shell test -d /opt/homebrew/ && echo y),y) + BASIC_CFLAGS += -I/opt/homebrew/include + BASIC_LDFLAGS += -L/opt/homebrew/lib + ifeq ($(shell test -x /opt/homebrew/bin/msgfmt && echo y),y) + MSGFMT = /opt/homebrew/bin/msgfmt + endif endif # The builtin FSMonitor on MacOS builds upon Simple-IPC. Both require From 99c60edc5b83cb624b30b8f459da78c250c63f87 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Thu, 18 Jan 2024 11:22:53 +0100 Subject: [PATCH 3/5] ci: handle TEST_OUTPUT_DIRECTORY when printing test failures The TEST_OUTPUT_DIRECTORY environment variable can be used to instruct the test suite to write test data and test results into a different location than into "t/". The "ci/print-test-failures.sh" script does not know to handle this environment variable though, which means that it will search for test results in the wrong location if it was set. Update the script to handle TEST_OUTPUT_DIRECTORY so that we can start to set it in our CI. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- ci/print-test-failures.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ci/print-test-failures.sh b/ci/print-test-failures.sh index c33ad4e3a2..b1f80aeac3 100755 --- a/ci/print-test-failures.sh +++ b/ci/print-test-failures.sh @@ -8,7 +8,7 @@ # Tracing executed commands would produce too much noise in the loop below. set +x -cd t/ +cd "${TEST_OUTPUT_DIRECTORY:-t/}" if ! ls test-results/*.exit >/dev/null 2>/dev/null then From c4b84b137ae7f18ac0fe30e2566725567b90ecca Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Thu, 18 Jan 2024 11:22:58 +0100 Subject: [PATCH 4/5] ci: make p4 setup on macOS more robust When setting up Perforce on macOS we put both `p4` and `p4d` into "$HOME/bin". On GitHub CI this directory is indeed contained in the PATH environment variable and thus there is no need for additional setup than to put the binaries there. But GitLab CI does not do this, and thus our Perforce-based tests would be skipped there even though we download the binaries. Refactor the setup code to become more robust by downloading binaries into a separate directory which we then manually append to our PATH. This matches what we do on Linux-based jobs. Note that it may seem like we already did append "$HOME/bin" to PATH because we're actually removing the lines that adapt PATH. But we only ever adapted the PATH variable in "ci/install-dependencies.sh", and didn't adapt it when running "ci/run-build-and-test.sh". Consequently, the required binaries wouldn't be found during the test run unless the CI platform already had the "$HOME/bin" in PATH right from the start. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- ci/install-dependencies.sh | 10 ++++------ ci/lib.sh | 3 +++ 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh index 4f407530d3..b4e22de3cb 100755 --- a/ci/install-dependencies.sh +++ b/ci/install-dependencies.sh @@ -37,15 +37,13 @@ macos-*) test -z "$BREW_INSTALL_PACKAGES" || brew install $BREW_INSTALL_PACKAGES brew link --force gettext - mkdir -p $HOME/bin - ( - cd $HOME/bin + + mkdir -p "$P4_PATH" + pushd "$P4_PATH" wget -q "$P4WHENCE/bin.macosx1015x86_64/helix-core-server.tgz" && tar -xf helix-core-server.tgz && sudo xattr -d com.apple.quarantine p4 p4d 2>/dev/null || true - ) - PATH="$PATH:${HOME}/bin" - export PATH + popd if test -n "$CC_PACKAGE" then diff --git a/ci/lib.sh b/ci/lib.sh index c749b21366..f631206a44 100755 --- a/ci/lib.sh +++ b/ci/lib.sh @@ -344,6 +344,9 @@ macos-*) then MAKEFLAGS="$MAKEFLAGS APPLE_COMMON_CRYPTO_SHA1=Yes" fi + + P4_PATH="$HOME/custom/p4" + export PATH="$P4_PATH:$PATH" ;; esac From 56090a35ab20c21ef577bd1ed2d9d5b63eb5f649 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Thu, 18 Jan 2024 11:23:02 +0100 Subject: [PATCH 5/5] ci: add macOS jobs to GitLab CI Add a job to GitLab CI which runs tests on macOS, which matches the equivalent "osx-clang" job that we have for GitHub Workflows. One significant difference though is that this new job runs on Apple M1 machines and thus uses the "arm64" architecture. As GCC does not yet support this comparatively new architecture we cannot easily include an equivalent for the "osx-gcc" job that exists in GitHub Workflows. Note that one test marked as `test_must_fail` is surprisingly passing: t7815-grep-binary.sh (Wstat: 0 Tests: 22 Failed: 0) TODO passed: 12 This seems to boil down to an unexpected difference in how regcomp(3P) works when matching NUL bytes. Cross-checking with the respective GitHub job shows that this is not an issue unique to the GitLab CI job as it passes in the same way there. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- .gitlab-ci.yml | 34 +++++++++++++++++++++++++++++++++- ci/lib.sh | 9 ++++++++- 2 files changed, 41 insertions(+), 2 deletions(-) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 793243421c..43bfbd8834 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -7,7 +7,7 @@ workflow: - if: $CI_COMMIT_TAG - if: $CI_COMMIT_REF_PROTECTED == "true" -test: +test:linux: image: $image before_script: - ./ci/install-docker-dependencies.sh @@ -52,6 +52,38 @@ test: - t/failed-test-artifacts when: on_failure +test:osx: + image: $image + tags: + - saas-macos-medium-m1 + variables: + TEST_OUTPUT_DIRECTORY: "/Volumes/RAMDisk" + before_script: + # Create a 4GB RAM disk that we use to store test output on. This small hack + # significantly speeds up tests by more than a factor of 2 because the + # macOS runners use network-attached storage as disks, which is _really_ + # slow with the many small writes that our tests do. + - sudo diskutil apfs create $(hdiutil attach -nomount ram://8192000) RAMDisk + - ./ci/install-dependencies.sh + script: + - ./ci/run-build-and-tests.sh + after_script: + - | + if test "$CI_JOB_STATUS" != 'success' + then + ./ci/print-test-failures.sh + mv "$TEST_OUTPUT_DIRECTORY"/failed-test-artifacts t/ + fi + parallel: + matrix: + - jobname: osx-clang + image: macos-13-xcode-14 + CC: clang + artifacts: + paths: + - t/failed-test-artifacts + when: on_failure + static-analysis: image: ubuntu:22.04 variables: diff --git a/ci/lib.sh b/ci/lib.sh index f631206a44..d5dd2f2697 100755 --- a/ci/lib.sh +++ b/ci/lib.sh @@ -252,7 +252,14 @@ then CI_COMMIT="$CI_COMMIT_SHA" case "$CI_JOB_IMAGE" in macos-*) - CI_OS_NAME=osx;; + # GitLab CI has Python installed via multiple package managers, + # most notably via asdf and Homebrew. Ensure that our builds + # pick up the Homebrew one by prepending it to our PATH as the + # asdf one breaks tests. + export PATH="$(brew --prefix)/bin:$PATH" + + CI_OS_NAME=osx + ;; alpine:*|fedora:*|ubuntu:*) CI_OS_NAME=linux;; *)