-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Revert the minimal golang version requirement from 1.17 to 1.16 and add a warning in Makefile #19319
Revert the minimal golang version requirement from 1.17 to 1.16 and add a warning in Makefile #19319
Conversation
…dd a warning in Makefile
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM after phrasing nits.
Co-authored-by: John Olheiser <john.olheiser@gmail.com>
@if [ "$(GO_VERSION)" -lt "$(MIN_GO_VERSION)" ]; then \ | ||
echo "Gitea requires Go $(MIN_GO_VERSION_STR) or greater to build. You can get it at https://go.dev/dl/"; \ | ||
echo "Gitea requires Go $(MIN_GO_VERSION_STR) or greater to build, but $(GO_VERSION) was found. You can get an updated version at https://go.dev/dl/"; \ | ||
exit 1; \ | ||
else \ | ||
echo "WARNING: Please ensure Go $(GO_VERSION_STR) is still maintained to avoid possible security problems. You can check it at https://go.dev/dl/"; \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't it very strange for a user who uses Go1.18 see this WARNING?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently yes, but when Golang go to 1.2x, it will not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think if we leave this, we can expect other issues with why this warning is appearing for users that are using the latest go version. We don't want to let them think that they are using a outdated go version when they are not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But we will never know it except we check Golang's website in the Makefile.
This actually won't build in 1.16 as this PR is now, due this file f9ea4ab#diff-93c289a98a6bd37986878061f982fad85e37b47dff818559e021216f9319a2a8 I will push changes to this branch to resolve before merging. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
temporarily blocking
Co-authored-by: Gusted <williamzijl7@hotmail.com>
yay tests for catching this :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Fix #19187