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

Revert the minimal golang version requirement from 1.17 to 1.16 and add a warning in Makefile #19319

Merged
merged 6 commits into from
Apr 5, 2022

Conversation

lunny
Copy link
Member

@lunny lunny commented Apr 4, 2022

Fix #19187

@lunny lunny added the topic/build PR changes how Gitea is built, i.e. regarding Docker or the Makefile label Apr 4, 2022
@lunny lunny added this to the 1.16.6 milestone Apr 4, 2022
@GiteaBot GiteaBot added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Apr 4, 2022
Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Copy link
Member

@jolheiser jolheiser left a 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.

@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 Apr 4, 2022
Co-authored-by: John Olheiser <john.olheiser@gmail.com>
Comment on lines 208 to +212
@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/"; \
Copy link
Contributor

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?

Copy link
Member Author

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.

Copy link
Contributor

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.

Copy link
Member Author

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.

@techknowlogick
Copy link
Member

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.

Copy link
Member

@techknowlogick techknowlogick left a comment

Choose a reason for hiding this comment

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

temporarily blocking

modules/util/net.go Outdated Show resolved Hide resolved
techknowlogick and others added 3 commits April 5, 2022 12:28
Co-authored-by: Gusted <williamzijl7@hotmail.com>
yay tests for catching this :)
@techknowlogick techknowlogick merged commit 0704009 into go-gitea:release/v1.16 Apr 5, 2022
Copy link
Member

@6543 6543 left a comment

Choose a reason for hiding this comment

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

lgtm

@go-gitea go-gitea locked and limited conversation to collaborators May 3, 2023
@lunny lunny deleted the lunny/min_go_version branch August 24, 2023 11:20
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. 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.

7 participants