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

Add missing cosmos-SDK release 0.25.0 version #2722

Closed
wants to merge 1 commit into from

Conversation

curicm
Copy link

@curicm curicm commented Nov 7, 2018

  • Targeted PR against correct branch (see CONTRIBUTING.md)

  • [n ] Linked to github-issue with discussion and accepted design OR link to spec that describes this work.

  • [+] Wrote tests

  • [n ] Updated relevant documentation (docs/)

  • [ n] Added entries in PENDING.md with issue #

  • [ n] rereviewed Files changed in the github PR explorer


For Admin Use:

  • Added appropriate labels to PR (ex. wip, ready-for-review, docs)
  • Reviewers Assigned
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

@alexanderbez
Copy link
Contributor

alexanderbez commented Nov 7, 2018

Hey @curicm! Thanks for the contribution. Do we need this when version is already set via:

VERSION := $(shell git describe --tags --long | sed 's/v\(.*\)/\1/')

which produces something along the lines of: v0.26.0-rc0-28-gf241a4b9

@codecov
Copy link

codecov bot commented Nov 7, 2018

Codecov Report

Merging #2722 into master will increase coverage by 0.02%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #2722      +/-   ##
==========================================
+ Coverage   58.81%   58.83%   +0.02%     
==========================================
  Files         152      152              
  Lines        9420     9420              
==========================================
+ Hits         5540     5542       +2     
+ Misses       3510     3508       -2     
  Partials      370      370

@curicm
Copy link
Author

curicm commented Nov 7, 2018

Hey @alexanderbez

I'm not sure. On current master branch for example: basecoin does not print version information:

$:~/go/src/github.com/cosmos/cosmos-sdk/examples/basecoin/cmd/basecli$ ./basecli version
<--[only empty line returned]

after the proposed change the version is displayed:

$./basecli version
0.25.0

This was working on previous cosmos-SDK and the version was set similarly in:
https://github.com/cosmos/cosmos-sdk/blob/v0.24.2/version/version.go

@alexanderbez
Copy link
Contributor

@curicm it was removed because we're not using git's describe mechanism, which is the more proper course of action (ref: #2318)

I just ran make on the latest develop and gaiad, gaiacli, and all the example binary all have the proper version set and displayed.

Perhaps you're using. Perhaps we simply didn't push tags to master? In any case, this code is not needed.

@curicm
Copy link
Author

curicm commented Nov 8, 2018

@alexanderbez thanks for pointing out ref: #2318

I see now where the problem is. Was building basecli with the automatically generated makefile from cosmos-sdk-cli binary. The proposed "git describe" change was added only to the main makefile, but not to the code in cosmos-sdk-cli which generates a custom makefile.
Since cosmos-sdk-cli is part of offical cosmos-SDK, probalby would be good to add this proper version setting mechanism there also.

the generated build command looks like this:

build:
go build -o bin/mybasecoincli cmd/mybasecoincli/main.go && go build -o bin/mybasecoind cmd/mybasecoind/main.go

@alexanderbez
Copy link
Contributor

Great catch @curicm! Would you like to make a PR for this fix? I will be closing this.

@curicm
Copy link
Author

curicm commented Nov 8, 2018

Fine. I can do a PR and will reference this conversation. Thanks.

@alexanderbez
Copy link
Contributor

Thanks @curicm 🍍

chillyvee pushed a commit to chillyvee/cosmos-sdk that referenced this pull request Mar 1, 2024
* fix md links

* remove proto docs and Makefile target

* fix md links
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