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

make: fix make install not using ldflags for git hash #3838

Merged
merged 1 commit into from
Mar 30, 2017

Conversation

Kubuxu
Copy link
Member

@Kubuxu Kubuxu commented Mar 27, 2017

Resolves #3837

License: MIT
Signed-off-by: Jakub Sztandera kubuxu@protonmail.ch

License: MIT
Signed-off-by: Jakub Sztandera <kubuxu@protonmail.ch>
@Kubuxu Kubuxu added the status/in-progress In progress label Mar 27, 2017
@Kubuxu Kubuxu added this to the Ipfs 0.4.9 milestone Mar 27, 2017
@whyrusleeping
Copy link
Member

@Kubuxu could we perhaps have a test for this?

@Kubuxu
Copy link
Member Author

Kubuxu commented Mar 27, 2017

Not sure how to test it, it uses buildsystem and at the same time it installs into GOPATH, it would be hard.

@whyrusleeping
Copy link
Member

A sharness test should be able to just check ipfs version --commit no?

@Kubuxu
Copy link
Member Author

Kubuxu commented Mar 27, 2017

This affects make install not make build. We test for commit in docker tests, not sure if they run.

I will add test for commit, but it will find regression with build not install and I can already tell that it won't pass with coverage collection (as coverage doesn't insert this variable either).

@whyrusleeping
Copy link
Member

@Kubuxu how about we default the variable to something that indicates its not an 'official' build?

@Kubuxu
Copy link
Member Author

Kubuxu commented Mar 27, 2017

So do we want to confront Semver?

Then I suggest:
Official 0.4.8-rc1
Unofficial if not git var available 0.4.8-rc1+dirty.
With git: 0.4.8-rc1+git.a8b56d3ad

@whyrusleeping
Copy link
Member

those SGTM. The 'sometag' is generally 'dirty'

@Kubuxu
Copy link
Member Author

Kubuxu commented Mar 27, 2017

We can also add +dirty if there is git hash but there are local (not committed changes)

@whyrusleeping
Copy link
Member

@Kubuxu is this ready to go?

@Kubuxu
Copy link
Member Author

Kubuxu commented Mar 30, 2017

If tests are to be separate, yes.

@whyrusleeping whyrusleeping merged commit aac4088 into master Mar 30, 2017
@whyrusleeping whyrusleeping deleted the fix/make/install-hash branch March 30, 2017 22:59
@whyrusleeping whyrusleeping removed the status/in-progress In progress label Mar 30, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants