Improve code format during build, remove `code-batch-process` #21471

wxiaoguang · 2022-10-16T02:05:46Z

The code-batch-process was introduce to resolve

"make fmt" fails on windows #14438

Actually, after chore(lint): use golangci-lint to call revive and misspell checker. #18145 , the misspell sub-command is never used, the only sub-command call to code-batch-process is gitea-fmt (the import formatter), no misspell anymore.

This PR ~~removes unused misspell package~~ (update: the misspell package is updated in #21458), introduce a gitea-fmt.sh to do the code format (the Makefile has been changed to depend on POSIX commands heavily, so the bash script should be no problem).

Without code-batch-process, some commands like gofumpt don't have the ability to ignore some specialized files. If the files are made to be ignored by Makefile, then the too-long-command-argument problem comes again.

I kept the old Makefile logic as much as possible, feel free to edit on this PR directly.

# Conflicts: # Makefile # build/code-batch-process.go # build/codeformat/formatimports.go

silverwind · 2022-10-18T06:31:00Z

Without code-batch-process, some commands like gofumpt don't have the ability to ignore some specialized files. If the files are made to be ignored by Makefile, then the too-long-command-argument problem comes again.

It appears gofumpt can accept directories as arguments, which could vastly reduce the command line length if we pass directories that do not contain any ignored files as-is. Only for directories with files to ignore, we'd need to rely on Make filter-out and friends.

silverwind · 2022-10-18T06:39:39Z

I feel like gofumpt should support a -exclude argument on CLI, I opened mvdan/gofumpt#250.

silverwind · 2022-10-18T06:40:55Z

build/gitea-fmt.sh

+GO_VERSION=$(grep -Eo '^go\s+[0-9]+\.[0-9]+' go.mod | cut -d' ' -f2)
+
+echo "Run gofumpt with Go language version $GO_VERSION ..."
+gofumpt -extra -lang "$GO_VERSION" "$1" .


It should use the version specified in the Makefile, not the globally installed one. So you would still need to pass GOFUMPT_PACKAGE to this script.

I would like to assume the the gofump has been installed by line 810 $(GO) install $(GOFUMPT_PACKAGE)

Otherwise the line 810 should be removed.

Feel free to propose changes to this PR.

It could still run a different version based on what the user had installed last unless go run is used. These go install lines only ensure that the specific version is present on the system. Execution should always be via go run with the specific version, otherwise you get the version the user had installed last.

After some more thinking, I think I would prefer to use pre-installed gofumpt (with correct version)

The reason is: Gitea had better to manage all its dependent binaries with correct version and have them in local cache. Otherwise, if developer's network is not stable or they are offline, the go run will be time wasting for their make fmt work.

Not sure what you mean with "pre-installed" but relying on the global gofumpt is dangerous. Say for example gitea uses gofumpt 0.3.1 and I work on another project which uses gofumpt 0.4.0 which happens to also install the global. The next time I format gitea, it will be with wrong formatting.

Stuff like that can be solved by just using the explicit local version. npm solved this problem around 5 years ago with npx which prefers a local over a global tool.

I mean something like you said, "manage all its dependent binaries with correct version and have them in local cache", like ./.../bin/gofumpt-0.x.x

silverwind · 2022-10-18T10:31:19Z

Makefile

@@ -264,7 +264,7 @@ clean:

 .PHONY: fmt
 fmt:
-	GOFUMPT_PACKAGE=$(GOFUMPT_PACKAGE) $(GO) run build/code-batch-process.go gitea-fmt -w '{file-list}'
+	@GO=$(GO) ./build/gitea-fmt.sh -w


Suggested change

@GO=$(GO) ./build/gitea-fmt.sh -w

@GOFUMPT_PACKAGE=$(GOFUMPT_PACKAGE) GO=$(GO) ./build/gitea-fmt.sh -w

silverwind · 2022-10-18T10:32:42Z

build/gitea-fmt.sh

+GO_VERSION=$(grep -Eo '^go\s+[0-9]+\.[0-9]+' go.mod | cut -d' ' -f2)
+
+echo "Run gofumpt with Go language version $GO_VERSION ..."
+gofumpt -extra -lang "$GO_VERSION" "$1" .


Suggested change

gofumpt -extra -lang "$GO_VERSION" "$1" .

$GO run "$GOFUMPT_PACKAGE" -extra -lang "$GO_VERSION" "$1" .

More discussions in #21471 (comment)

silverwind · 2022-10-18T10:34:43Z

build/gitea-fmt.sh

+gofumpt -extra -lang "$GO_VERSION" "$1" .
+
+echo "Run codeformat ..."
+$GO run ./build/codeformat.go "$1" .


I think this will fail if $GO path contains whitespace, but I generally think we shouldn't need this variable in first place and we can just use plain go from PATH.

Ah, if I remember the MAKEFILE syntax correctly, there is one small difference: GO ?= go should allow custom go paths by overriding the env var, and it simply defaults to the Go in your path.

I think this will fail if $GO path contains whitespace, but I generally think we shouldn't need this variable in first place and we can just use plain go from PATH.

Fixed in a new commit.

Ah, if I remember the MAKEFILE syntax correctly, there is one small difference: GO ?= go should allow custom go paths by overriding the env var, and it simply defaults to the Go in your path.

In line 5, GO="${GO:=go}, the Makefile could also pass the GO environment to the script.

build/gitea-fmt.sh

silverwind · 2022-10-19T12:04:04Z

I think we would be better of also removing build/codeformat.go and make make fmt be a simple invocation of $GOFUMPT_PACKAGE with directory arguments where possible which should reduce the argument length enough to fit within the 8k limit of Windows.

wxiaoguang · 2022-10-30T07:56:36Z

It seems that there is still no clear solution. I will close this PR and defer it to the future.

This was referenced Oct 16, 2022

chore(lint): use golangci-lint to call revive and misspell checker. #18145

Merged

Use own Go version instead of hardcoded 1.17 for make fmt #21457

Merged

wxiaoguang changed the title ~~Improve code format during build, remove code-batch-format~~ Improve code format during build, remove code-batch-process Oct 16, 2022

improve codeformat

179a960

wxiaoguang force-pushed the improve-codeformat branch from 834ca8d to 179a960 Compare October 16, 2022 02:11

wxiaoguang added 2 commits October 16, 2022 16:12

Merge branch 'main' into improve-codeformat

aaa12e1

# Conflicts: # Makefile # build/code-batch-process.go # build/codeformat/formatimports.go

fix merge

71a84a1

wxiaoguang mentioned this pull request Oct 16, 2022

Simplify fmt-check #21458

Merged

wxiaoguang added this to the 1.18.0 milestone Oct 17, 2022

wxiaoguang added the topic/build PR changes how Gitea is built, i.e. regarding Docker or the Makefile label Oct 17, 2022

lunny approved these changes Oct 18, 2022

View reviewed changes

GiteaBot added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Oct 18, 2022

silverwind reviewed Oct 18, 2022

View reviewed changes

wxiaoguang commented Oct 18, 2022

View reviewed changes

build/gitea-fmt.sh Outdated Show resolved Hide resolved

Update build/gitea-fmt.sh

35a36d0

wxiaoguang removed this from the 1.18.0 milestone Oct 19, 2022

wxiaoguang closed this Oct 30, 2022

go-gitea locked and limited conversation to collaborators May 3, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve code format during build, remove `code-batch-process` #21471

Improve code format during build, remove `code-batch-process` #21471

wxiaoguang commented Oct 16, 2022 •

edited

Loading

silverwind commented Oct 18, 2022

silverwind commented Oct 18, 2022

silverwind Oct 18, 2022 •

edited

Loading

wxiaoguang Oct 18, 2022

silverwind Oct 18, 2022 •

edited

Loading

wxiaoguang Oct 18, 2022 •

edited

Loading

silverwind Oct 18, 2022

wxiaoguang Oct 19, 2022

silverwind Oct 18, 2022 •

edited

Loading

silverwind Oct 18, 2022 •

edited

Loading

wxiaoguang Oct 18, 2022

silverwind Oct 18, 2022

delvh Oct 18, 2022

wxiaoguang Oct 18, 2022 •

edited

Loading

silverwind commented Oct 19, 2022 •

edited

Loading

wxiaoguang commented Oct 30, 2022

	@GO=$(GO) ./build/gitea-fmt.sh -w
	@GOFUMPT_PACKAGE=$(GOFUMPT_PACKAGE) GO=$(GO) ./build/gitea-fmt.sh -w

	gofumpt -extra -lang "$GO_VERSION" "$1" .
	$GO run "$GOFUMPT_PACKAGE" -extra -lang "$GO_VERSION" "$1" .

Improve code format during build, remove code-batch-process #21471

Improve code format during build, remove code-batch-process #21471

Conversation

wxiaoguang commented Oct 16, 2022 • edited Loading

silverwind commented Oct 18, 2022

silverwind commented Oct 18, 2022

silverwind Oct 18, 2022 • edited Loading

Choose a reason for hiding this comment

wxiaoguang Oct 18, 2022

Choose a reason for hiding this comment

silverwind Oct 18, 2022 • edited Loading

Choose a reason for hiding this comment

wxiaoguang Oct 18, 2022 • edited Loading

Choose a reason for hiding this comment

silverwind Oct 18, 2022

Choose a reason for hiding this comment

wxiaoguang Oct 19, 2022

Choose a reason for hiding this comment

silverwind Oct 18, 2022 • edited Loading

Choose a reason for hiding this comment

silverwind Oct 18, 2022 • edited Loading

Choose a reason for hiding this comment

wxiaoguang Oct 18, 2022

Choose a reason for hiding this comment

silverwind Oct 18, 2022

Choose a reason for hiding this comment

delvh Oct 18, 2022

Choose a reason for hiding this comment

wxiaoguang Oct 18, 2022 • edited Loading

Choose a reason for hiding this comment

silverwind commented Oct 19, 2022 • edited Loading

wxiaoguang commented Oct 30, 2022

Improve code format during build, remove `code-batch-process` #21471

Improve code format during build, remove `code-batch-process` #21471

wxiaoguang commented Oct 16, 2022 •

edited

Loading

silverwind Oct 18, 2022 •

edited

Loading

silverwind Oct 18, 2022 •

edited

Loading

wxiaoguang Oct 18, 2022 •

edited

Loading

silverwind Oct 18, 2022 •

edited

Loading

silverwind Oct 18, 2022 •

edited

Loading

wxiaoguang Oct 18, 2022 •

edited

Loading

silverwind commented Oct 19, 2022 •

edited

Loading