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

Update IAVL dependency for v0.10.0 #1952

Merged
merged 7 commits into from Sep 6, 2018
Merged

Update IAVL dependency for v0.10.0 #1952

merged 7 commits into from Sep 6, 2018

Conversation

UnitylChaos
Copy link
Contributor

  • Linked to github-issue with discussion and accepted design OR link to spec that describes this work.
  • Updated all relevant documentation (docs/)
  • Updated all relevant code comments
  • Wrote tests
  • Added entries in PENDING.md that include links to the relevant issue or PR that most accurately describes the change.
  • Updated cmd/gaia and examples/

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)

@codecov
Copy link

codecov bot commented Aug 9, 2018

Codecov Report

Merging #1952 into develop will not change coverage.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           develop   #1952   +/-   ##
=======================================
  Coverage     64.2%   64.2%           
=======================================
  Files          140     140           
  Lines         8579    8579           
=======================================
  Hits          5508    5508           
  Misses        2694    2694           
  Partials       377     377

@alexanderbez
Copy link
Contributor

alexanderbez commented Aug 13, 2018

@jlandrews this can finally be updated + merged? 🙏

@UnitylChaos
Copy link
Contributor Author

Should be able to be merged very soon, just need to cut a release on IAVL that this can reference.

@liamsi liamsi mentioned this pull request Aug 14, 2018
Gopkg.toml Outdated
@@ -53,7 +53,7 @@

[[override]]
name = "github.com/tendermint/iavl"
version = "=v0.9.2"
revision = "bab6ea5006ebf4e97e918eadcd6ea338e6d415eb"
Copy link
Contributor

Choose a reason for hiding this comment

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

You can use version = "=v0.10.0" as soon as cosmos/iavl#100 is merged and tagged on master.

@jackzampolin
Copy link
Member

Looks like we can pin this to a version and merge this now @jlandrews

@UnitylChaos
Copy link
Contributor Author

Not sure what's going on with the linter here since none of that code is being changed... Should be good to go other than that though.

@UnitylChaos UnitylChaos changed the title WIP - Change types for IAVL refactor Update IAVL dependency for v0.10.0 Aug 15, 2018
@cwgoes
Copy link
Contributor

cwgoes commented Aug 20, 2018

We can ignore the linter but this should have a changelog (pending) entry.

Copy link
Contributor

@cwgoes cwgoes left a comment

Choose a reason for hiding this comment

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

utACK

Copy link
Contributor

@cwgoes cwgoes left a comment

Choose a reason for hiding this comment

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

Let's investigate these linter failures a bit, this is strange, merging develop didn't fix them but lint passes fine on develop.

@ValarDragon
Copy link
Contributor

ValarDragon commented Aug 23, 2018

Let's first debug if this is something going on with circle or the lint script. Does make test lint pass on develop locally?

@UnitylChaos UnitylChaos force-pushed the jlandrews/immutable-avl branch 2 times, most recently from 20a8bc1 to 6b3c0a3 Compare August 28, 2018 01:56
@UnitylChaos
Copy link
Contributor Author

Really confused as to what's going on here. I tried making a new branch and doing the changes piecemeal to see where it breaks, but unfortunately that didn't work since the minimal change that builds is basically the entire thing. I tried running make test_lint on develop locally and got:

jlandrews@jlandrews-lpt ~/go/src/github.com/cosmos/cosmos-sdk $ make test_lint
gometalinter.v2 --config=tools/gometalinter.json ./...
!(gometalinter.v2 --disable-all --enable='errcheck' --vendor ./... | grep -v "client/")
find . -name '*.go' -type f -not -path "./vendor*" -not -path "*.git*" | xargs gofmt -d -s
diff -u ./server/export_test.go.orig ./server/export_test.go
--- ./server/export_test.go.orig        2018-08-27 19:29:14.324955161 -0700
+++ ./server/export_test.go     2018-08-27 19:29:14.324955161 -0700
@@ -1,16 +1,16 @@
 package server

 import (
-       "testing"
-       "github.com/stretchr/testify/require"
+       "bytes"
+       "github.com/cosmos/cosmos-sdk/server/mock"
        "github.com/cosmos/cosmos-sdk/wire"
-       "github.com/tendermint/tendermint/libs/log"
+       "github.com/stretchr/testify/require"
        tcmd "github.com/tendermint/tendermint/cmd/tendermint/commands"
-       "os"
-       "bytes"
+       "github.com/tendermint/tendermint/libs/log"
        "io"
-               "github.com/cosmos/cosmos-sdk/server/mock"
-       )
+       "os"
+       "testing"
+)

 func TestEmptyState(t *testing.T) {
        defer setupViper(t)()
diff -u ./server/mock/app.go.orig ./server/mock/app.go
--- ./server/mock/app.go.orig   2018-08-27 19:29:14.328955179 -0700
+++ ./server/mock/app.go        2018-08-27 19:29:14.328955179 -0700
@@ -129,7 +129,7 @@

 // Return a validator, not much else
 func AppGenTx(_ *wire.Codec, pk crypto.PubKey, genTxConfig gc.GenTx) (
-               appGenTx, cliPrint json.RawMessage, validator tmtypes.GenesisValidator, err error) {
+       appGenTx, cliPrint json.RawMessage, validator tmtypes.GenesisValidator, err error) {

        validator = tmtypes.GenesisValidator{
                PubKey: pk,
dep status >> /dev/null
!(grep -n branch Gopkg.toml)

I don't know if that's the expected output there.

It's worth noting that the lint errors on this branch seem to be real lint violations that are present in develop, so maybe the most straightforward course of action would be to just fix the errors. That leaves open the question of why these errors aren't getting seen on develop, and what change in this branch is causing them to be seen, but I can't seem to figure out what what mechanism is.

Posting lint errors here for reference:

#!/bin/bash -eo pipefail
export PATH="$GOBIN:$PATH"
make test_lint
gometalinter.v2 --config=tools/gometalinter.json ./...
baseapp/baseapp_test.go:629:25:warning: unnecessary conversion (unconvert)
baseapp/baseapp_test.go:634:25:warning: unnecessary conversion (unconvert)
store/tracekvstore_test.go:159:26:warning: unnecessary conversion (unconvert)
store/tracekvstore_test.go:160:26:warning: unnecessary conversion (unconvert)
store/tracekvstore_test.go:215:26:warning: unnecessary conversion (unconvert)
store/tracekvstore_test.go:216:26:warning: unnecessary conversion (unconvert)
x/slashing/keeper_test.go:105:24:warning: unnecessary conversion (unconvert)
x/slashing/keeper_test.go:135:24:warning: unnecessary conversion (unconvert)
x/slashing/keeper_test.go:192:24:warning: unnecessary conversion (unconvert)
x/slashing/keeper_test.go:236:24:warning: unnecessary conversion (unconvert)
x/slashing/keeper_test.go:244:24:warning: unnecessary conversion (unconvert)
x/stake/client/rest/query.go:275:37:warning: unnecessary conversion (unconvert)
x/stake/client/rest/query.go:334:37:warning: unnecessary conversion (unconvert)
x/stake/handler_test.go:342:35:warning: unnecessary conversion (unconvert)
baseapp/baseapp.go:348:50:warning: parameter handleQueryP2P - req is unused (unparam)
cmd/cosmos-sdk-cli/cmd/init.go:63:27:warning: parameter copyBasecoinTemplate - projectName is unused (unparam)
cmd/cosmos-sdk-cli/cmd/init.go:63:67:warning: parameter copyBasecoinTemplate - remoteProjectPath is unused (unparam)
server/init.go:347:23:warning: parameter writeGenesisFile - cdc is unused (unparam)
x/gov/queryable.go:39:37:warning: parameter queryProposal - path is unused (unparam)
x/gov/queryable.go:64:36:warning: parameter queryDeposit - path is unused (unparam)
x/gov/queryable.go:85:33:warning: parameter queryVote - path is unused (unparam)
x/gov/queryable.go:105:37:warning: parameter queryDeposits - path is unused (unparam)
x/gov/queryable.go:132:34:warning: parameter queryVotes - path is unused (unparam)
x/gov/queryable.go:163:38:warning: parameter queryProposals - path is unused (unparam)
x/gov/queryable.go:184:34:warning: parameter queryTally - path is unused (unparam)
x/ibc/client/cli/relay.go:166:37:warning: parameter (relayCommander).broadcastTx - seq is unused (unparam)
x/stake/keeper/key.go:71:55:warning: parameter getValidatorPowerRank - pool is unused (unparam)
x/stake/keeper/slash.go:196:52:warning: parameter (Keeper).slashRedelegation - validator is unused (unparam)
x/stake/keeper/validator.go:334:36:warning: parameter (Keeper).getPowerIncreasing - ctx is unused (unparam)
x/stake/keeper/validator.go:343:2:warning: parameter (Keeper).bondIncrement - newValidator is unused (unparam)
Makefile:165: recipe for target 'test_lint' failed
make: *** [test_lint] Error 1
Exited with code 2

@cwgoes
Copy link
Contributor

cwgoes commented Aug 29, 2018

I am equally confused, I guess we should just fix the linter errors.

@ValarDragon
Copy link
Contributor

I'd opt for just fixing linting errors here. Something funky is going on, however the linter is detecting these issues on this branch. The worst case situation is that some bug is causing the linter to not get executed in some circumstances, which isn't that big of a deal. I think debugging any more situations similar to this can be moved to #postlaunch.

@jlandrews can you also be sure to merge in develop, since there have been a lot of updates.

@UnitylChaos UnitylChaos force-pushed the jlandrews/immutable-avl branch 2 times, most recently from 36ff038 to 672f999 Compare August 31, 2018 22:32
PENDING.md Outdated

* [store] \#1952 Update IAVL dependency to v0.10.0

BUG FIXES
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you fix the pending file and put the improvements line in the already allocated section?

@@ -111,6 +113,7 @@ func createGopkg(projectPath string) {
ioutil.WriteFile(projectPath+"/Gopkg.toml", []byte(contents), os.ModePerm)
}

// nolint: errcheck
Copy link
Contributor

Choose a reason for hiding this comment

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

Lets make a follow-up issue to handle the nolints?

Copy link
Contributor

@ValarDragon ValarDragon left a comment

Choose a reason for hiding this comment

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

lgtm, just needs that changelog fix

@cwgoes
Copy link
Contributor

cwgoes commented Sep 3, 2018

Return of the linter failures?

@UnitylChaos
Copy link
Contributor Author

No, these appear to be new lint errors introduced by rebasing off develop. Looks like there are recent changes that have unconvert violations. I can fix these too. I still find it curious that this branch / IAVL update somehow enables the linter to catch these while it doesn't on other branches...

@ValarDragon
Copy link
Contributor

Can we get this merged, would be good to have for next release imo. With that in mind we can do more debugging the sooner it gets in!

@alexanderbez
Copy link
Contributor

alexanderbez commented Sep 5, 2018

++

@cwgoes cwgoes merged commit e410a9e into develop Sep 6, 2018
@cwgoes cwgoes deleted the jlandrews/immutable-avl branch September 6, 2018 09:18
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.

None yet

6 participants