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

bump to a newer tendermint version #112

Closed
wants to merge 3 commits into from
Closed

bump to a newer tendermint version #112

wants to merge 3 commits into from

Conversation

tac0turtle
Copy link
Contributor

Description

  • brings LL to a later Tendermint

Closes: #XXX

@tac0turtle tac0turtle mentioned this pull request Dec 11, 2020
@lgtm-com
Copy link

lgtm-com bot commented Dec 11, 2020

This pull request fixes 10 alerts when merging 517035e into 7ae7b13 - view on LGTM.com

fixed alerts:

  • 7 for Illegal raise
  • 3 for Unused import

Comment on lines +49 to +51
- name: Build Tendermint
run: |
make build-linux && cp build/tendermint DOCKER/tendermint
Copy link
Member

Choose a reason for hiding this comment

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

I didn't try this. Will this still work due to the renaming?

Copy link
Member

@liamsi liamsi left a comment

Choose a reason for hiding this comment

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

Thanks so much @marbar3778! Love to see that the v2 blockchain reactor received some attention recently. I'll go through all the changes shortly. Awesome work!

with:
go-version: '^1.15.4'

- run: echo https://github.com/tendermint/tendermint/blob/${GITHUB_REF#refs/tags/}/CHANGELOG.md#${GITHUB_REF#refs/tags/} > ../release_notes.md
Copy link
Member

Choose a reason for hiding this comment

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

Should be renamed. In fact, we will very likely prep the first releases manually anyway though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will comment it out for now then.

- id: "Tendermint"
main: ./cmd/tendermint/main.go
ldflags:
- -s -w -X github.com/tendermint/tendermint/version.TMCoreSemVer={{ .Version }}
Copy link
Member

Choose a reason for hiding this comment

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

Should be renamed too.

ldflags:
- -s -w -X github.com/tendermint/tendermint/version.TMCoreSemVer={{ .Version }}
env:
- CGO_ENABLED=0
Copy link
Member

Choose a reason for hiding this comment

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

I guess our releases will use CGO to compile the go-leopard library.

@@ -4,7 +4,8 @@ PACKAGES=$(shell go list ./...)
BUILDDIR ?= $(CURDIR)/build

BUILD_TAGS?=tendermint
LD_FLAGS = -X github.com/tendermint/tendermint/version.GitCommit=`git rev-parse --short=8 HEAD`
VERSION := $(shell git describe --always)
LD_FLAGS = -X github.com/tendermint/tendermint/version.TMCoreSemVer=$(VERSION)
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be renamed too?

github.com/confio/ics23/go v0.0.0-20200817220745-f173e6211efb
github.com/cosmos/iavl v0.15.0-rc3.0.20201009144442-230e9bdf52cd
github.com/confio/ics23/go v0.6.3
github.com/cosmos/iavl v0.15.0
Copy link
Member

Choose a reason for hiding this comment

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

Wasn't iavl removed as a dependency from tendermint?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope it was added back for testing. We have an open issue to solve this: tendermint/tendermint#5694

golang.org/x/crypto v0.0.0-20200820211705-5c72a883971a
golang.org/x/net v0.0.0-20200813134508-3edf25e44fcc
google.golang.org/grpc v1.32.0
github.com/tendermint/tendermint v0.34.0
Copy link
Member

Choose a reason for hiding this comment

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

It's a bit odd that tendermint is a dep here. I think this is due to a test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will groom through to find out why it's still here.

@liamsi
Copy link
Member

liamsi commented Dec 12, 2020

One general remark/question: shouldn't we rather merge the changes from upstream as commits instead of 'squashing' all changes into one commit (3707bf4)? The former would give credit to the original authors (and git blame is more useful that way too). Also, the git histories would be more alike 🤔

@tac0turtle
Copy link
Contributor Author

One general remark/question: shouldn't we rather merge the changes from upstream as commits instead of 'squashing' all changes into one commit (3707bf4)? The former would give credit to the original authors (and git blame is more useful that way too). Also, the git histories would be more alike 🤔

Oh I didnt mean for this to happen. Will undo it, it may be through a new pr though.

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