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

resolved #91: automatically generate gitea version #101

Closed
wants to merge 3 commits into from

Conversation

lunny
Copy link
Member

@lunny lunny commented Nov 7, 2016

And I create the make.bash for linux and mac, we need another make.bat for windows. Of course, this is a beginning of make bash. I think a bash file to make is simple and better than Makefile.

@codecov-io
Copy link

codecov-io commented Nov 7, 2016

Current coverage is 2.24% (diff: 100%)

Merging #101 into master will not change coverage

@@            master      #101   diff @@
========================================
  Files           32        32          
  Lines         7521      7521          
  Methods          0         0          
  Messages         0         0          
  Branches         0         0          
========================================
  Hits           169       169          
  Misses        7335      7335          
  Partials        17        17          

Powered by Codecov. Last update 5a6f7ed...c5ecdc8

@tboerger
Copy link
Member

tboerger commented Nov 7, 2016

I personally prefer a makefile, most of the bash content is done there.

version="unknow"

if [ -f VERSION ]; then
cat /etc/passwd | read version
Copy link
Member

@tboerger tboerger Nov 7, 2016

Choose a reason for hiding this comment

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

passwd?


if [ "$version" != "HEAD" ]; then
if [ "$version" == "master" ]; then
go build -ldflags "-X main.Version=tip+${tag}"
Copy link
Member

Choose a reason for hiding this comment

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

I'm not a fan of tip as a "version"

@makhov
Copy link
Contributor

makhov commented Nov 7, 2016

What's wrong with Makefile? Why we need make.bash?

@lunny
Copy link
Member Author

lunny commented Nov 7, 2016

You have to install extra make system. But for a bash, it has no more dependency.

@tboerger
Copy link
Member

tboerger commented Nov 7, 2016

To be serious, I think nearly all people working with Go got make installed. I personally don't really want to maintain 2 files for a single use case.

@tboerger
Copy link
Member

tboerger commented Nov 7, 2016

And if we change the version assignment I would also drop https://github.com/go-gitea/gitea/blob/master/modules/setting/setting.go#L51-L52 and the places where it gets used.

# grep -R BuildGitHash *
Makefile:15:LDFLAGS += -X "github.com/go-gitea/gitea/modules/setting.BuildGitHash=$(SHA)"
modules/setting/setting.go:52:  BuildGitHash string
modules/setting/setting.go:620:     log.Info("Build Git Hash: %s", BuildGitHash)
# grep -R BuildTime *
Makefile:14:LDFLAGS += -X "github.com/go-gitea/gitea/modules/setting.BuildTime=$(DATE)"
modules/setting/setting.go:51:  BuildTime    string
modules/setting/setting.go:618: if len(BuildTime) > 0 {
modules/setting/setting.go:619:     log.Info("Build Time: %s", BuildTime)

@tboerger
Copy link
Member

tboerger commented Nov 7, 2016

And this is my suggestion for the Makefile to get it working based on this proposal, I would always keep a valid version number, tip is not a version number and with a plain 0.0.0 we can still always see that this have been built from master:

--- Makefile.current    2016-11-07 09:25:49.668272844 +0100
+++ Makefile    2016-11-07 09:33:32.880272024 +0100
@@ -4,31 +4,30 @@
 EXECUTABLE := gitea
 IMPORT := github.com/go-gitea/gitea

-SHA := $(shell git rev-parse --short HEAD)
-DATE := $(shell date -u '+%Y-%m-%d %I:%M:%S %Z')
-
 BINDATA := $(shell find conf | sed 's/ /\\ /g')
 STYLESHEETS := $(wildcard public/less/index.less public/less/_*.less)
 JAVASCRIPTS :=

-LDFLAGS += -X "github.com/go-gitea/gitea/modules/setting.BuildTime=$(DATE)"
-LDFLAGS += -X "github.com/go-gitea/gitea/modules/setting.BuildGitHash=$(SHA)"
+REF ?= $(shell git rev-parse --abbrev-ref HEAD)
+SHA ?= $(shell git describe --tag --always)

 TARGETS ?= linux/*,darwin/*,windows/*
 PACKAGES ?= $(shell go list ./... | grep -v /vendor/)

 TAGS ?=

-ifneq ($(TRAVIS_TAG),)
-   VERSION ?= $(TRAVIS_TAG)
+ifneq ($(REF),HEAD)
+   VERSION ?= 0.0.0+$(SHA)
 else
-   ifneq ($(TRAVIS_BRANCH),)
-       VERSION ?= $(TRAVIS_BRANCH)
+   ifneq ($(REF),master)
+       VERSION ?= 0.0.0+$(SHA)
    else
-       VERSION ?= master
+       VERSION ?= $(REF)+$(SHA)
    endif
 endif

+LDFLAGS += -X "main.Version=$(VERSION)"
+
 .PHONY: all
 all: clean test build

@tboerger tboerger added this to the 1.0.0 milestone Nov 7, 2016
@tboerger tboerger added the type/proposal The new feature has not been accepted yet but needs to be discussed first. label Nov 7, 2016
@strk
Copy link
Member

strk commented Nov 7, 2016

What's wrong with just using SHA ? Why do we need REF too ? Do you have example resulting VERSION strings ?

@tboerger
Copy link
Member

tboerger commented Nov 7, 2016

REF is used to detect if it's HEAD, master or a tag, SHA just adds the commit hash. So of course we need both if we want to detect the version automatically.

@strk
Copy link
Member

strk commented Nov 7, 2016

And why do we want to distinguish, was my question.
When calling git describe --always --tag while in a tag, I do get the tag as the version, which is what we want, while being not in a tag would give you the most recent tag + a count of commits + the actual hash, what else are we missing from that ?

@tboerger
Copy link
Member

tboerger commented Nov 7, 2016

We WANT to have master=0.0.0|HEAD=0.0.0|tag=1.0.0+commit-sha, maybe we also want to strip the v from the tag name.

@strk
Copy link
Member

strk commented Nov 7, 2016

On Mon, Nov 07, 2016 at 04:15:17AM -0800, Thomas Boerger wrote:

We WANT to have master=0.0.0|HEAD=0.0.0|tag=1.0.0+commit-sha, maybe we also want to strip the v from the tag name.

I don't get this, why don't you always want an unambiguous
version number, including the "commit-sha" ?

@tboerger
Copy link
Member

tboerger commented Nov 7, 2016

We want a proper version scheme that applies for package managers.

@strk
Copy link
Member

strk commented Nov 7, 2016

Maybe I misunderstood your suggestion, as long as the +commit-sha is always appended (also for master/HEAD) I'm ok with the proposal

@@ -17,7 +17,7 @@ import (
)

// Version holds the current Gitea version
const Version = "0.9.99.0915"
var Version = "0.9.99.0915"
Copy link
Member

Choose a reason for hiding this comment

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

I would set that to var Version = "0.0.0+master"

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 still be const, since it shouldn't be changed at runtime

Copy link
Member

Choose a reason for hiding this comment

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

If I remember correctly you can not manipulate const from outside? Need to confirm that.

Copy link
Member Author

Choose a reason for hiding this comment

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

A const can not be set via go build flag.

@strk
Copy link
Member

strk commented Nov 7, 2016 via email

@lunny lunny modified the milestones: 1.x.x, 1.0.0 Nov 8, 2016
@tboerger
Copy link
Member

Let's close that in favor of #190

@tboerger tboerger closed this Nov 24, 2016
@tboerger tboerger added lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. issue/duplicate The issue has already been reported. and removed type/proposal The new feature has not been accepted yet but needs to be discussed first. labels Nov 24, 2016
@tboerger tboerger removed the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Nov 24, 2016
@tboerger tboerger removed this from the 1.x.x milestone Nov 24, 2016
@lunny lunny deleted the build_script branch April 19, 2017 05:46
lunny pushed a commit to lunny/gitea that referenced this pull request Feb 7, 2019
@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
issue/duplicate The issue has already been reported.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants