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

fix: Stop overriding umask with test code #4296

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

hugoh
Copy link

@hugoh hugoh commented Feb 22, 2025

Fixes #4214.

init() in chezmoitest_unix.go was being executed even for the released version.

Now, doctor gives the correct umask:

❯ umask 077
❯ go run . doctor | grep umask
warning   umask                       077

instead of always returning 022.

I ended up using a build tag to include it only on test builds. There likely is a cleaner way to do this.

@hugoh hugoh changed the title fix: stop overriding umask with test code fix: Stop overriding umask with test code Feb 22, 2025
Copy link
Collaborator

@halostatue halostatue Feb 22, 2025

Choose a reason for hiding this comment

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

I am not sure that adding -tags=test is the right choice here1, but it is the minimal file change option. However, there are more files that need changing than you may have realized, and I disagree with your choice of popping the init function into its own file.

Below, you’ll see a .patch file that I came up with in looking at this and addressing this a little more directly. It includes:

  • .github/workflows/main.yml, Makefile: These should be the same changes as yours.
  • assets/chezmoi.io/docs/developer-guide/architecture.md,assets/chezmoi.io/docs/developer-guide/index.md
  • assets/vagrant/freebsd14.test-chezmoi.sh, assets/vagrant/openbsd7.test-chezmoi.sh

I approached the init() problem differently than you did in internal/chezmoitest/chezmoitest.go, internal/chezmoitest/chezmoitest_unix.go, and internal/chezmoitest/chezmoitest_windows.go.

Fundamentally, I think that this is a go compiler error. chezmoitest is its own package and is never imported into chezmoi itself except in _test files (which are not included in non test builds). As such, any init blocks should not be linked into the resulting binary but they clearly are. I’m not going to chase this, but I think it would be worthwhile coming up with a minimal reproduction and reporting it to the Go team.


changes.patch:

diff --git c/.github/workflows/main.yml i/.github/workflows/main.yml
index 2319526f43bc..52ff8c44f066 100644
--- c/.github/workflows/main.yml
+++ i/.github/workflows/main.yml
@@ -164,18 +164,18 @@ jobs:
     - name: test
       env:
         CHEZMOI_GITHUB_TOKEN: ${{ secrets.CHEZMOI_GITHUB_TOKEN }}
       run: |
         if [ "${{ matrix.test-index }}" = "0" ]; then
-          go test ./... -short -race
-          go test ./internal/cmd -run=TestScript -filter='^[0-9a-dA-D]' -race
+          go test -tags=test ./... -short -race
+          go test -tags=test ./internal/cmd -run=TestScript -filter='^[0-9a-dA-D]' -race
         elif [ "${{ matrix.test-index }}" = "1" ]; then
-          go test ./internal/cmd -run=TestScript -filter='^[e-hE-H]' -race
+          go test -tags=test ./internal/cmd -run=TestScript -filter='^[e-hE-H]' -race
         elif [ "${{ matrix.test-index }}" = "2" ]; then
-          go test ./internal/cmd -run=TestScript -filter='^[i-lI-L]' -race
+          go test -tags=test ./internal/cmd -run=TestScript -filter='^[i-lI-L]' -race
         else
-          go test ./internal/cmd -run=TestScript -filter='^[m-zM-Z]' -race
+          go test -tags=test ./internal/cmd -run=TestScript -filter='^[m-zM-Z]' -race
         fi
   test-oldstable-go:
     needs: changes
     if: github.event_name == 'push' || needs.changes.outputs.code == 'true'
     runs-on: ubuntu-22.04
@@ -206,11 +206,11 @@ jobs:
         sudo install -m 755 rage/rage-keygen /usr/local/bin
     - name: test
       env:
         CHEZMOI_GITHUB_TOKEN: ${{ secrets.CHEZMOI_GITHUB_TOKEN }}
       run: |
-        go test ./...
+        go test -tags=test ./...
   test-release:
     needs: changes
     runs-on: ubuntu-22.04 # use older Ubuntu for older glibc
     permissions:
       contents: read
@@ -321,14 +321,14 @@ jobs:
       env:
         CHEZMOI_GITHUB_TOKEN: ${{ secrets.CHEZMOI_GITHUB_TOKEN }}
         TEST_FLAGS: '-ldflags="-X github.com/twpayne/chezmoi/v2/internal/chezmoitest.umaskStr=0o${{ matrix.umask }}" -race -timeout=1h'
       run: |
         if [ "${{ matrix.test-index }}" = "0" ]; then
-          go test ./... -short ${{ env.TEST_FLAGS }}
-          go test ./internal/cmd -run=TestScript -filter='^[0-9a-hA-h]' ${{ env.TEST_FLAGS }}
+          go test -tags=test ./... -short ${{ env.TEST_FLAGS }}
+          go test -tags=test ./internal/cmd -run=TestScript -filter='^[0-9a-hA-h]' ${{ env.TEST_FLAGS }}
         else
-          go test ./internal/cmd -run=TestScript -filter='^[i-zI-Z]' ${{ env.TEST_FLAGS }}
+          go test -tags=test ./internal/cmd -run=TestScript -filter='^[i-zI-Z]' ${{ env.TEST_FLAGS }}
         fi
   test-website:
     runs-on: ubuntu-22.04
     permissions:
       contents: read
@@ -375,18 +375,18 @@ jobs:
     - name: test
       env:
         CHEZMOI_GITHUB_TOKEN: ${{ secrets.CHEZMOI_GITHUB_TOKEN }}
       run: |
         if (${{ matrix.test-index }} -eq 0) {
-          go test ./... -short -race
-          go test ./internal/cmd -run=TestScript -filter='^[0-9a-cA-C]' -race
+          go test -tags=test ./... -short -race
+          go test -tags=test ./internal/cmd -run=TestScript -filter='^[0-9a-cA-C]' -race
         } elseif (${{ matrix.test-index }} -eq 1) {
-          go test ./internal/cmd -run=TestScript -filter='^[d-fD-F]' -race
+          go test -tags=test ./internal/cmd -run=TestScript -filter='^[d-fD-F]' -race
         } elseif (${{ matrix.test-index }} -eq 2) {
-          go test ./internal/cmd -run=TestScript -filter='^[g-lG-L]' -race
+          go test -tags=test ./internal/cmd -run=TestScript -filter='^[g-lG-L]' -race
         } else {
-          go test ./internal/cmd -run=TestScript -filter='^[m-zM-Z]' -race
+          go test -tags=test ./internal/cmd -run=TestScript -filter='^[m-zM-Z]' -race
         }
   check:
     runs-on: ubuntu-22.04
     permissions:
       contents: read
diff --git c/Makefile i/Makefile
index 421bd9d5c3a9..bf3c4d696e0d 100644
--- c/Makefile
+++ i/Makefile
@@ -89,12 +89,12 @@ test-all: test test-release rm-dist test-docker test-vagrant
 rm-dist:
 	rm -rf dist
 
 .PHONY: test
 test:
-	${GO} test -ldflags="-X github.com/twpayne/chezmoi/v2/internal/chezmoitest.umaskStr=0o022" ./...
-	${GO} test -ldflags="-X github.com/twpayne/chezmoi/v2/internal/chezmoitest.umaskStr=0o002" ./...
+	${GO} test -tags=test -ldflags="-X github.com/twpayne/chezmoi/v2/internal/chezmoitest.umaskStr=0o022" ./...
+	${GO} test -tags=test -ldflags="-X github.com/twpayne/chezmoi/v2/internal/chezmoitest.umaskStr=0o002" ./...
 
 .PHONY: test-docker
 test-docker:
 	( cd assets/docker && ./test.sh alpine archlinux fedora )
 
diff --git c/assets/chezmoi.io/docs/developer-guide/architecture.md i/assets/chezmoi.io/docs/developer-guide/architecture.md
index cf413be8220e..d99f541041c8 100644
--- c/assets/chezmoi.io/docs/developer-guide/architecture.md
+++ i/assets/chezmoi.io/docs/developer-guide/architecture.md
@@ -156,20 +156,20 @@ in the `entryState` bucket along with its contents SHA256 sum. On future
 invocations the script is only run if its contents SHA256 sum has changed, and
 its contents SHA256 sum is then updated in the persistent state.
 
 ## Testing
 
-chezmoi has a mix of, unit, integration, and end-to-end tests. Unit and
+chezmoi has a mix of unit, integration, and end-to-end tests. Unit and
 integration tests use the [`github.com/alecthomas/assert/v2`][assert] framework.
 End-to-end tests use [`github.com/rogpeppe/go-internal/testscript`][testscript]
 with the test scripts themselves in
 `internal/cmd/testdata/scripts/$TEST_NAME.txtar`.
 
 You can run individual end-to-end tests with
 
 ```sh
-go test ./internal/cmd -run=TestScript/$TEST_NAME
+go test -tags=test ./internal/cmd -run=TestScript/$TEST_NAME
 ```
 
 Tests should, if at all possible, run unmodified on all operating systems tested
 in CI (Linux, macOS, Windows, and FreeBSD). Windows will sometimes need special
 handling due to its path separator and lack of POSIX-style file permissions.
diff --git c/assets/chezmoi.io/docs/developer-guide/index.md i/assets/chezmoi.io/docs/developer-guide/index.md
index 44dfa21ee329..b15660170f8e 100644
--- c/assets/chezmoi.io/docs/developer-guide/index.md
+++ i/assets/chezmoi.io/docs/developer-guide/index.md
@@ -8,12 +8,12 @@
     correctness before sharing it. If you share un-reviewed LLM-generated
     content then you will be immediately banned. See [`CODE_OF_CONDUCT.md`][coc]
     for more information.
 
 chezmoi is written in [Go][go] and development happens on [GitHub][github].
-chezmoi is a standard Go project, using standard Go tooling. chezmoi requires
-Go 1.24 or later.
+chezmoi is a standard Go project, using standard Go tooling. chezmoi requires Go
+1.24 or later.
 
 Checkout chezmoi:
 
 ```sh
 git clone https://github.com/twpayne/chezmoi.git
@@ -27,11 +27,11 @@ go build
 ```
 
 Run all tests:
 
 ```sh
-go test ./...
+go test -tags=test ./...
 ```
 
 chezmoi's tests include integration tests with other software. If the other
 software is not found in `$PATH` the tests will be skipped. Running the full set
 of tests requires `age`, `base64`, `bash`, `bzip2`, `git`, `gpg`, `gzip`,
@@ -71,11 +71,11 @@ make test-release
     These can be avoided with by running tests with `SHELL=bash` or `SHELL=zsh`:
 
     ```sh
     SHELL=bash make test
     SHELL=zsh make smoke-test
-    SHELL=bash go test ./...
+    SHELL=bash go test -tags=test ./...
     ```
 
 [coc]: https://github.com/twpayne/chezmoi/blob/master/.github/CODE_OF_CONDUCT.md
 [go]: https://golang.org
 [github]: https://github.com
diff --git c/assets/vagrant/freebsd14.test-chezmoi.sh i/assets/vagrant/freebsd14.test-chezmoi.sh
index 47566250a8ed..b035047d6f4d 100755
--- c/assets/vagrant/freebsd14.test-chezmoi.sh
+++ i/assets/vagrant/freebsd14.test-chezmoi.sh
@@ -4,9 +4,9 @@ set -eufo pipefail
 
 git config --global --add safe.directory /chezmoi
 
 cd /chezmoi
 
-go test ./...
+go test -tags=test ./...
 
 sh assets/scripts/install.sh
 bin/chezmoi --version
diff --git c/assets/vagrant/openbsd7.test-chezmoi.sh i/assets/vagrant/openbsd7.test-chezmoi.sh
index 47566250a8ed..b035047d6f4d 100755
--- c/assets/vagrant/openbsd7.test-chezmoi.sh
+++ i/assets/vagrant/openbsd7.test-chezmoi.sh
@@ -4,9 +4,9 @@ set -eufo pipefail
 
 git config --global --add safe.directory /chezmoi
 
 cd /chezmoi
 
-go test ./...
+go test -tags=test ./...
 
 sh assets/scripts/install.sh
 bin/chezmoi --version
diff --git c/internal/chezmoitest/chezmoitest.go i/internal/chezmoitest/chezmoitest.go
index 09b1110b92af..68bf87f87c5f 100644
--- c/internal/chezmoitest/chezmoitest.go
+++ i/internal/chezmoitest/chezmoitest.go
@@ -18,11 +18,15 @@
 	"github.com/twpayne/go-vfs/v5/vfst"
 
 	"github.com/twpayne/chezmoi/v2/internal/chezmoilog"
 )
 
-var ageRecipientRx = regexp.MustCompile(`(?m)^Public key: ([0-9a-z]+)\s*$`)
+var (
+	ageRecipientRx = regexp.MustCompile(`(?m)^Public key: ([0-9a-z]+)\s*$`)
+
+	Umask fs.FileMode
+)
 
 // AgeGenerateKey generates an identity in identityFile and returns the
 // recipient.
 func AgeGenerateKey(command, identityFile string) (string, error) {
 	cmd := exec.Command(command+"-keygen", "--output", identityFile)
diff --git c/internal/chezmoitest/chezmoitest_unix.go i/internal/chezmoitest/chezmoitest_unix.go
index f1cb8a756b81..beb08d9afc1a 100644
--- c/internal/chezmoitest/chezmoitest_unix.go
+++ i/internal/chezmoitest/chezmoitest_unix.go
@@ -1,29 +1,28 @@
-//go:build unix
+//go:build unix && test
 
 package chezmoitest
 
 import (
 	"golang.org/x/sys/unix"
 )
 
-var (
-	// umaskStr is the umask used in tests represented as a string so it can be
-	// set with the
-	//   -ldflags="-X github.com/twpayne/chezmoi/v2/internal/chezmoitest.umaskStr=..."
-	// option to go build and go test.
-	umaskStr = "0o022"
+// umaskStr is the umask used in tests represented as a string so it can be
+// set with the
+//
+//	-ldflags="-X github.com/twpayne/chezmoi/v2/internal/chezmoitest.umaskStr=..."
+//
+// option to go build and go test.
+var umaskStr = "0o022"
 
+func init() {
 	// Umask is the umask used in tests.
 	//
 	// If you change this then you will need to update the testscripts in
 	// testdata/scripts where permissions after applying umask are hardcoded as
 	// strings. Pure Go tests should use this value to ensure that they pass,
 	// irrespective of what it is set to. Be aware that the process's umask is a
 	// process-level property and cannot be locally changed within individual
 	// tests.
 	Umask = mustParseFileMode(umaskStr)
-)
-
-func init() {
 	unix.Umask(int(Umask))
 }
diff --git c/internal/chezmoitest/chezmoitest_windows.go i/internal/chezmoitest/chezmoitest_windows.go
index 65c0b2f3de0d..3fa92469b5bb 100644
--- c/internal/chezmoitest/chezmoitest_windows.go
+++ i/internal/chezmoitest/chezmoitest_windows.go
@@ -1,14 +1,18 @@
+//go:build windows && test
+
 package chezmoitest
 
-var (
-	// umaskStr is the umask used in tests represented as a string so it can be
-	// set with the
-	//   -ldflags="-X github.com/twpayne/chezmoi/v2/internal/chezmoitest.umaskStr=..."
-	// option to go build and go test.
-	umaskStr = "0"
+// umaskStr is the umask used in tests represented as a string so it can be
+// set with the
+//
+//	-ldflags="-X github.com/twpayne/chezmoi/v2/internal/chezmoitest.umaskStr=..."
+//
+// option to go build and go test.
+var umaskStr = "0"
 
+func init() {
 	// Umask is the umask used in tests.
 	//
 	// On Windows, Umask is zero as Windows does not use POSIX-style permissions.
 	Umask = mustParseFileMode(umaskStr)
-)
+}

Footnotes

  1. I would prefer //go:build !release, but that would mean adding go build -tags=release to all release builds and goreleaser both, which would mean modifying, well, every other file in the build. This could have been resolved with proposal: add a build tag "test" golang/go#21360, but purity over practicality, I guess. Equally annoying is the fact that you can't just use //go:build test on all files in internal/chezmoitest without breaking the entire build because the build tags exclude all files in that directory. Yes, that's on purpose, so just don't build that!

@halostatue halostatue requested a review from twpayne February 22, 2025 22:03
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.

git-repo external and umask don't play nice
2 participants