distribution: Lock binary dependencies to specific commits #2550

ValarDragon · 2018-10-05T07:10:40Z

Pulling revisions was relegated to scripts/get_tools.sh
The solution there isn't that elegant, but it works.
A more elegant + clean solution would be using python, but
we probably don't want to introduce that as a dependency.

I think we can leave it right now as a shell file with redundant
code, and improve the build process #postlaunch.

The linters which gometalinters are versioned according to gometalinters
install.sh. Its not locking to commit (AFAIK), but I think this is fine
as that is purely dev tooling and doesn't affect the code.
EDIT: It does lock to commit

Closes #2238

Updated all relevant documentation in docs - n/a?
Updated all code comments where relevant
Wrote tests - n/a
Updated CHANGELOG_PENDING.md

Pulling revisions was relegated to scripts/get_tools.sh The solution there isn't that elegant, but it works. A more elegant + clean solution would be using python, but we probably don't want to introduce that as a dependency. I think we can leave it right now as a shell file with redundant code, and improve the build process #postlaunch. The linters which gometalinters are versioned according to gometalinters install.sh. Its not locking to commit (AFAIK), but I think this is fine as that is purely dev tooling and doesn't affect the code. Closes #2238

melekes

🍰 🌮 🍉

melekes · 2018-10-05T07:17:31Z

scripts/get_tools.sh

+# github.com/gogo/protobuf/protoc-gen-gogo
+# github.com/square/certstrap
+
+cd $GOPATH/src/github.com


pushd $GOPATH/src/github.com

This didn't work in shell scripts for me, though it works in bash

nvm it worked after specifying the shell, thanks!

melekes · 2018-10-05T07:17:49Z

scripts/get_tools.sh

+go build
+cd ../../
+
+cd $GOPATH/src/github.com/tendermint/tendermint


melekes · 2018-10-05T07:19:31Z

scripts/get_tools.sh

+cd $GOPATH/src/github.com
+
+## install gox
+mkdir -p mitchellh


I'd prefer multiline style

git checkout -q 51ed453898ca5579fea9ad1f08dff6b121d9f2e8 && \ go build && \ cd ../../

I find that more confusing personally, but I'll change it if you like :)

melekes · 2018-10-05T07:21:38Z

scripts/get_tools.sh

+## install protoc-gen-gogo
+mkdir -p gogo
+cd gogo
+if cd protobuf; then git fetch origin; else git clone https://github.com/gogo/protobuf.git; cd protobuf; fi


if [ ! -d "protobuf" ]; then git clone ...; fi

no need for fetching imho if you later checkout specific commit

Don't you need to fetch if you already have the repo cloned, but the commit we specify is later than what you have?

Oh, you're right. Anyway, [ -d "...] syntax is more common for bash scripts

scripts/get_tools.sh

melekes · 2018-10-05T07:23:13Z

Makefile

-	@gometalinter.v2 --install
+	./scripts/get_tools.sh
+	@echo "--> Downloading linters (this may take awhile)"
+	./../../alecthomas/gometalinter/scripts/install.sh -b $(GOBIN)


can we use absolute path here? $GOPATH/...

melekes · 2018-10-05T07:24:30Z

Makefile

@@ -1,7 +1,7 @@
 GOTOOLS = \
 	github.com/mitchellh/gox \
 	github.com/golang/dep/cmd/dep \
-	gopkg.in/alecthomas/gometalinter.v2 \


@zramsay what was the reason to use v2?

locking to the latest version

This checks out the v2.0.11 release (their latest release), which was being used anyway.

zramsay · 2018-10-05T12:36:19Z

Makefile

@@ -1,7 +1,7 @@
 GOTOOLS = \
 	github.com/mitchellh/gox \
 	github.com/golang/dep/cmd/dep \
-	gopkg.in/alecthomas/gometalinter.v2 \
+	github.com/alecthomas/gometalinter \


this will pull latest master, which can cause non-determinism in our linting

Metalinter is still hash locked in the script file. You only get different versions with update tools. (Though I could make update tools just call get tools as well)

codecov-io · 2018-10-06T02:01:40Z

Codecov Report

Merging #2550 into develop will increase coverage by 0.03%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           develop    #2550      +/-   ##
===========================================
+ Coverage    61.23%   61.26%   +0.03%     
===========================================
  Files          202      202              
  Lines        16733    16734       +1     
===========================================
+ Hits         10246    10252       +6     
+ Misses        5617     5612       -5     
  Partials       870      870

Impacted Files	Coverage Δ
libs/db/prefix_db.go	`56.54% <0%> (-1.2%)`	⬇️
evidence/reactor.go	`65.93% <0%> (+0.5%)`	⬆️
consensus/reactor.go	`71.18% <0%> (+0.64%)`	⬆️
libs/events/events.go	`96.07% <0%> (+2.87%)`	⬆️
crypto/encoding/amino/amino.go	`85.71% <0%> (+2.95%)`	⬆️

ValarDragon

LGTM! Thanks for doing this! My bash-fu wasn't up to par to figure this out lol. Github won't let me approve this b/c I made the PR though

ValarDragon requested review from ebuchman, melekes and xla as code owners October 5, 2018 07:10

melekes reviewed Oct 5, 2018

View reviewed changes

zramsay reviewed Oct 5, 2018

View reviewed changes

ValarDragon added 2 commits October 5, 2018 18:28

Address comments

f17b620

use go install, not go build

253b855

ebuchman mentioned this pull request Oct 6, 2018

Deterministic build: Lock binary dependencies #2238

Closed

ValarDragon and others added 2 commits October 5, 2018 19:50

Make update_tools get the correct versions

1f09135

Merge branch 'develop' into dev/lock_deps

f3c780d

melekes force-pushed the dev/lock_deps branch from f7236d6 to a76172c Compare October 9, 2018 15:08

refactor get_tools script

af1f734

melekes force-pushed the dev/lock_deps branch from a76172c to af1f734 Compare October 9, 2018 15:21

check if the subdirectory exists

f402f6f

melekes approved these changes Oct 9, 2018

View reviewed changes

ValarDragon commented Oct 9, 2018

View reviewed changes

xla changed the title ~~Lock binary dependencies to specific commits~~ distribution: Lock binary dependencies to specific commits Oct 9, 2018

xla merged commit d7341c4 into develop Oct 9, 2018

xla deleted the dev/lock_deps branch October 9, 2018 17:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

distribution: Lock binary dependencies to specific commits #2550

distribution: Lock binary dependencies to specific commits #2550

ValarDragon commented Oct 5, 2018 •

edited

melekes left a comment

melekes Oct 5, 2018

ValarDragon Oct 5, 2018

ValarDragon Oct 6, 2018

melekes Oct 5, 2018

melekes Oct 5, 2018

ValarDragon Oct 6, 2018

melekes Oct 5, 2018

ValarDragon Oct 5, 2018

melekes Oct 6, 2018

melekes Oct 5, 2018

melekes Oct 5, 2018

zramsay Oct 5, 2018

ValarDragon Oct 6, 2018

zramsay Oct 5, 2018

ValarDragon Oct 5, 2018

codecov-io commented Oct 6, 2018 •

edited

ValarDragon left a comment

distribution: Lock binary dependencies to specific commits #2550

distribution: Lock binary dependencies to specific commits #2550

Conversation

ValarDragon commented Oct 5, 2018 • edited

melekes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Oct 6, 2018 • edited

Codecov Report

ValarDragon left a comment

Choose a reason for hiding this comment

ValarDragon commented Oct 5, 2018 •

edited

codecov-io commented Oct 6, 2018 •

edited