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

Check go and nodejs version by go.mod and package.json, update Go official site URL #19197

Merged
merged 12 commits into from
Mar 26, 2022

Conversation

gesangtome
Copy link
Contributor

Looks like upstream has removed support built
with GO 1.16, the string should be updated here.

Looks like upstream has removed support built 
with GO 1.16, the string should be updated here.
@wxiaoguang
Copy link
Contributor

wxiaoguang commented Mar 24, 2022

Maybe we can remove the whole go-check and MIN_GO_VERSION, because the go.mod can take the work.

go mod tidy: go.mod file indicates go 1.17, but maximum supported version is 1.16

gitea/go.mod

Line 3 in def5456

go 1.17

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Mar 24, 2022
Copy link
Member

@silverwind silverwind left a comment

Choose a reason for hiding this comment

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

Change is good in any case, it was missed in fe9626a

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Mar 24, 2022
@silverwind
Copy link
Member

silverwind commented Mar 24, 2022

Maybe we can remove the whole go-check and MIN_GO_VERSION, because the go.mod can take the work.

I think go.mod is meant to declare the current (e.g. maximum) version, not the minimum one.

@wxiaoguang
Copy link
Contributor

I think go.mod is meant to declare the current (e.g. maximum) version, not the minimum one.

Why? Then why can I compile Gitea with my Go1.18 ..........

@silverwind
Copy link
Member

silverwind commented Mar 24, 2022

Actually, I'm wrong, it specifies minimum as per https://go.dev/doc/modules/gomod-ref#go, so it might be an option to just parse it out of go.mod, but I think it's material for another PR.

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Mar 24, 2022

Why another? Go will report the correct required version automatically by go.mod now, what's the necessary of the old age go-check?

@silverwind
Copy link
Member

silverwind commented Mar 24, 2022

Will go fail to build if the version is too low? If yes, than we can actually remove the check.

@gesangtome
Copy link
Contributor Author

gesangtome commented Mar 24, 2022

Doesn't gitea have a minimum go version requirement?
@silverwind @wxiaoguang

@wxiaoguang
Copy link
Contributor

Doesn't gitea have a minimum go version requirement?

Yes, only the official supported Golang releases (the latest 2 minor versions) are supported.

Will go fail to build if the version is too low? If yes, than we can actually remove the check.

Starting from 1.16, the go build could report correct messages. However, Go 1.15 or older can not report correct messages.

So I think it's fine to remove the go-check, because Go1.15 is quite old now.

And it's also fine to keep the go-check, and maybe the version can be read from the go.mod, then reduce a magic number in Makefile.

$ /usr/local/go-1.16/bin/go build
# code.gitea.io/gitea/modules/hostmatcher
modules/hostmatcher/hostmatcher.go:105:34: ip.IsPrivate undefined (type net.IP has no field or method IsPrivate)
modules/hostmatcher/hostmatcher.go:109:9: ip.IsPrivate undefined (type net.IP has no field or method IsPrivate)
note: module requires Go 1.17

$ /usr/local/go-1.15/bin/go build
../../go/pkg/mod/github.com/caddyserver/[email protected]/filestorage.go:23:2: package io/fs is not in GOROOT (/usr/local/go-1.15/src/io/fs)

@silverwind
Copy link
Member

And it's also fine to keep the go-check, and maybe the version can be read from the go.mod, then reduce a magic number in Makefile.

Yes, I support this. Same can be done for node-check as well which defines its minimum version in package.json, but I guess parsing it without additional dependencies will be a bit ugly.

@wxiaoguang
Copy link
Contributor

I updated the go version parsing, and tested locally.

@gesangtome
Copy link
Contributor Author

And it's also fine to keep the go-check, and maybe the version can be read from the go.mod, then reduce a magic number in Makefile.

Yes, I support this. Same can be done for node-check as well which defines its minimum version in package.json, but I guess parsing it without additional dependencies will be a bit ugly.

Make go-check emit version hints when GO < 1.16?
If yes, we can do this:

diff --git a/Makefile b/Makefile
index 613c08326..5c066fd3b 100644
--- a/Makefile
+++ b/Makefile
@@ -25,7 +25,7 @@ HAS_GO = $(shell hash $(GO) > /dev/null 2>&1 && echo "GO" || echo "NOGO" )
COMMA := ,

XGO_VERSION := go-1.18.x
-MIN_GO_VERSION := 001017000
+MIN_GO_VERSION := 001016000
MIN_NODE_VERSION := 012017000

AIR_PACKAGE ?= github.com/cosmtrek/[email protected]
@@ -205,7 +205,7 @@ help:
go-check:
$(eval GO_VERSION := $(shell printf "%03d%03d%03d" $(shell $(GO) version | grep -Eo '[0-9]+.[0-9.]+' | tr '.' ' ');))
@if [ "$(GO_VERSION)" -lt "$(MIN_GO_VERSION)" ]; then \

  •           echo "Gitea requires Go 1.16 or greater to build. You can get it at https://golang.org/dl/"; \
    
  •           echo "Sorry, your GO version is too low, Gitea requests an updated GO to build. You can get it at https://golang.org/dl/"; \
              exit 1; \
      fi
    

@wxiaoguang
Copy link
Contributor

@silverwind I also make this PR use the nodejs version from package.json

@wxiaoguang wxiaoguang changed the title Update string for go-check Check go and nodejs version by go.mod and package.json, update Go official site URL Mar 24, 2022
@wxiaoguang wxiaoguang added topic/build PR changes how Gitea is built, i.e. regarding Docker or the Makefile skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. labels Mar 24, 2022
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Mar 24, 2022
@silverwind silverwind self-requested a review March 24, 2022 15:30
Makefile Outdated Show resolved Hide resolved
Makefile Outdated
MIN_GO_VERSION_STR := $(shell grep -Eo '^go\s+[0-9]+\.[0-9.]+' go.mod | cut -d' ' -f2)
MIN_GO_VERSION := $(shell printf "%03d%03d%03d" $(shell echo '$(MIN_GO_VERSION_STR)' | tr '.' ' '))

MIN_NODE_VERSION_STR := $(shell grep -Eo '"node":.*[0-9.]+"' package.json | sed -n 's/.*[^0-9.]\([0-9.]*\)"/\1/p')
Copy link
Member

@silverwind silverwind Mar 24, 2022

Choose a reason for hiding this comment

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

May as well extract the third version segment as well because node version is always 3-segments as opposed to go version which is 2.

Copy link
Contributor

Choose a reason for hiding this comment

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

It already has the 3-segments. The regex is [0-9.]*

Makefile Outdated
MIN_NODE_VERSION := 012017000

MIN_GO_VERSION_STR := $(shell grep -Eo '^go\s+[0-9]+\.[0-9.]+' go.mod | cut -d' ' -f2)
MIN_GO_VERSION := $(shell printf "%03d%03d%03d" $(shell echo '$(MIN_GO_VERSION_STR)' | tr '.' ' '))
Copy link
Member

Choose a reason for hiding this comment

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

The line above extracts 2 segments, but this seems to output 3. Is it correct?

Copy link
Contributor

@wxiaoguang wxiaoguang Mar 24, 2022

Choose a reason for hiding this comment

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

It already has the 3-segments. The regex is [0-9.]*

@silverwind
Copy link
Member

Also, I strongly suggest not doing such shell operation on top-level make scope because they will execute whenever the Makefile is invoked which is both wasteful and may produce errors in case when node is absent. It should only be done in eval statements which execute only when the specific target runs.

@wxiaoguang
Copy link
Contributor

Also, I strongly suggest not doing such shell operation on top-level make scope because they will execute whenever the Makefile is invoked which is both wasteful and may produce errors in case when node is absent. It should only be done in eval statements which execute only when the specific target runs.

Moved these lines into go-check and node-check

@wxiaoguang wxiaoguang requested a review from silverwind March 24, 2022 16:52
@gesangtome
Copy link
Contributor Author

@wxiaoguang @silverwind

Things are going in a good direction, great, looking forward to it being fixed.

@codecov-commenter
Copy link

Codecov Report

Merging #19197 (948516d) into main (49c5fc5) will increase coverage by 0.02%.
The diff coverage is 39.03%.

@@            Coverage Diff             @@
##             main   #19197      +/-   ##
==========================================
+ Coverage   46.55%   46.58%   +0.02%     
==========================================
  Files         856      858       +2     
  Lines      123018   123128     +110     
==========================================
+ Hits        57277    57365      +88     
- Misses      58814    58833      +19     
- Partials     6927     6930       +3     
Impacted Files Coverage Δ
cmd/admin.go 0.00% <0.00%> (ø)
cmd/hook.go 7.11% <0.00%> (ø)
cmd/serv.go 2.39% <0.00%> (ø)
cmd/web_acme.go 0.00% <0.00%> (ø)
models/asymkey/ssh_key_deploy.go 55.55% <ø> (+1.13%) ⬆️
models/helper_environment.go 100.00% <ø> (ø)
modules/context/permission.go 25.39% <0.00%> (ø)
modules/doctor/checkOldArchives.go 22.85% <0.00%> (ø)
modules/doctor/fix16961.go 35.06% <0.00%> (ø)
modules/doctor/mergebase.go 10.25% <0.00%> (ø)
... and 198 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 59b867d...948516d. Read the comment docs.

@wxiaoguang wxiaoguang merged commit c119828 into go-gitea:main Mar 26, 2022
zjjhot added a commit to zjjhot/gitea that referenced this pull request Mar 26, 2022
* giteaofficial/main:
  Check go and nodejs version by go.mod and package.json (go-gitea#19197)
  Add `ContextUser` to http request context (go-gitea#18798)
  Set OpenGraph title to DisplayName in profile pages (go-gitea#19206)
Chianina pushed a commit to Chianina/gitea that referenced this pull request Mar 28, 2022
* Check go and nodejs version by go.mod and package.json 
* Update Go official site URL 

Co-authored-by: wxiaoguang <[email protected]>
wxiaoguang added a commit to wxiaoguang/gitea that referenced this pull request Mar 29, 2022
* Check go and nodejs version by go.mod and package.json 
* Update Go official site URL 

Co-authored-by: wxiaoguang <[email protected]>
wxiaoguang added a commit that referenced this pull request Mar 29, 2022
* Check go and nodejs version by go.mod and package.json 
* Update Go official site URL 

Co-authored-by: wxiaoguang <[email protected]>
Co-authored-by: gesangtome <[email protected]>
@go-gitea go-gitea locked and limited conversation to collaborators Apr 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. topic/build PR changes how Gitea is built, i.e. regarding Docker or the Makefile
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants