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 for #828: Embed build tags #1051

Merged
merged 5 commits into from
Feb 28, 2017
Merged

Conversation

cez81
Copy link
Contributor

@cez81 cez81 commented Feb 25, 2017

Add build tags to ldflags and print in version output

Add build tags to ldflags and print in version output

Signed-off-by: Jonas Östanbäck <jonas.ostanback@gmail.com>
@andreynering andreynering requested review from tboerger and removed request for tboerger February 25, 2017 12:56
@andreynering andreynering added topic/build PR changes how Gitea is built, i.e. regarding Docker or the Makefile type/enhancement An improvement of existing functionality labels Feb 25, 2017
@andreynering
Copy link
Contributor

/cc @tboerger

@tboerger tboerger added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Feb 25, 2017
@appleboy
Copy link
Member

LGTM

@tboerger tboerger 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 Feb 25, 2017
@lunny lunny added this to the 1.1.0 milestone Feb 25, 2017
@lunny
Copy link
Member

lunny commented Feb 25, 2017

seems great

@lunny
Copy link
Member

lunny commented Feb 25, 2017

will fix #828

main.go Outdated
if len(Tags) > 0 {
return " built with: " + strings.Join(strings.Split(Tags, " "), ", ")
}
return ""
Copy link
Member

@appleboy appleboy Feb 25, 2017

Choose a reason for hiding this comment

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

 func formatBuiltWith(Tags string) string {
  	if len(Tags) == 0 {
  		return ""
  	}

  	return " built with: " + strings.Join(strings.Split(Tags, " "), ", ")

more readable?

Copy link
Member

Choose a reason for hiding this comment

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

Or strings.Replace(Tags, " ", ", ", -1)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, is guess the special case is empty tags. Will update with both suggestions.

Signed-off-by: Jonas Östanbäck <jonas.ostanback@gmail.com>
@lunny
Copy link
Member

lunny commented Feb 26, 2017

It didn't work well. Because Version is also be set on the make progress. I think maybe you can only change the Makefile.

LDFLAGS := -X "main.Version=$(shell git describe --tags --always | sed 's/-/+/' | sed 's/^v//') $(TAGS)"

It's ok.

@cez81
Copy link
Contributor Author

cez81 commented Feb 26, 2017

@lunny Don't think I understand where else you mean it's used. Can you explain please?

@tboerger
Copy link
Member

I like how it's set now, but I'm not sure if I like how it's getting printed out

@tboerger
Copy link
Member

The version also gets printed on the page itself. Maybe we want to print the tags also on the admin page

@lunny
Copy link
Member

lunny commented Feb 27, 2017

@cez81 like @tboerger said, put it in admin panel maybe better.

main.go Outdated
}

return " built with: " + strings.Replace(Tags, " ", ", ", -1)
}
Copy link
Member

Choose a reason for hiding this comment

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

strings.Join

Copy link
Member

Choose a reason for hiding this comment

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

Tags is sqlite bindata form. Why use strings.Join ?

Copy link
Member

Choose a reason for hiding this comment

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

True, but maybe it's even easier to create a comma separated list to split by space and join by comma

@tboerger
Copy link
Member

I'm fine with displaying it as a comma separated list without quotes if I look on the referenced issue. But additionally it should be displayed on the admin ui.

Signed-off-by: Jonas Östanbäck <jonas.ostanback@gmail.com>
@cez81
Copy link
Contributor Author

cez81 commented Feb 27, 2017

tags_admin_panel
tags_console

main.go Outdated
func init() {
setting.AppVer = Version
setting.AppVer = Version + formatBuiltWith(Tags)
Copy link
Member

Choose a reason for hiding this comment

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

This should be done with a separate variable, IMHO we should not list the build tags on every page in the footer, only within the admin dashboard

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True, I missed that!

Signed-off-by: Jonas Östanbäck <jonas.ostanback@gmail.com>
@lunny
Copy link
Member

lunny commented Feb 27, 2017

build failed.

@@ -48,6 +48,9 @@ func NewFuncMap() []template.FuncMap {
"AppVer": func() string {
return setting.AppVer
},
"AppBuiltWith": func() string {
return setting.AppBuiltWith
},
Copy link
Member

Choose a reason for hiding this comment

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

coding style.

@tboerger
Copy link
Member

LGTM

@tboerger tboerger 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 Feb 27, 2017
@lunny lunny merged commit a201977 into go-gitea:master Feb 28, 2017
@cez81 cez81 deleted the embed_build_tags branch February 28, 2017 12:04
@go-gitea go-gitea locked and limited conversation to collaborators Nov 23, 2020
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 type/enhancement An improvement of existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants