Skip to content

arm: add install script, fix Makefile#2824

Merged
melekes merged 3 commits intodevelopfrom
zach/arm
Nov 14, 2018
Merged

arm: add install script, fix Makefile#2824
melekes merged 3 commits intodevelopfrom
zach/arm

Conversation

@zramsay
Copy link
Contributor

@zramsay zramsay commented Nov 13, 2018

  • Updated all relevant documentation in docs
  • Updated all code comments where relevant
  • Wrote tests
  • Updated CHANGELOG_PENDING.md

@melekes
Copy link
Contributor

melekes commented Nov 13, 2018 

#!/bin/bash -eo pipefail
set -ex
export PATH="$GOBIN:$PATH"
make metalinter
+ export PATH=/tmp/workspace/bin:/go/bin:/usr/local/go/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin
+ PATH=/tmp/workspace/bin:/go/bin:/usr/local/go/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin
+ make metalinter
--> Running linter
WARNING: exec: "deadcode": executable file not found in $PATH
WARNING: exec: "gosimple": executable file not found in $PATH
WARNING: exec: "misspell": executable file not found in $PATH
WARNING: exec: "safesql": executable file not found in $PATH
Makefile:231: recipe for target 'metalinter' failed
make: *** [metalinter] Error 2
Exited with code 2

liamsi
liamsi reviewed Nov 13, 2018
View reviewed changes
Makefile
@echo "--> Installing tools"
./scripts/get_tools.sh
@echo "--> Downloading linters (this may take awhile)"
$(GOPATH)/src/github.com/alecthomas/gometalinter/scripts/install.sh -b $(GOBIN)
Copy link
Contributor

@liamsi liamsi Nov 13, 2018

Choose a reason for hiding this comment

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

If you add back these two lines, this should resolve #2824 (comment) AFAIK.

Copy link
Contributor Author

@zramsay zramsay Nov 13, 2018

Choose a reason for hiding this comment

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

this line fails when installing on RPi: https://github.com/alecthomas/gometalinter/blob/master/scripts/install.sh#L62

should we add a platform check for this line?

Copy link
Contributor

@melekes melekes Nov 13, 2018

Choose a reason for hiding this comment

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

should we add a platform check for this line?

yes (with a comment saying it does not work on ...)

Copy link
Contributor Author

@zramsay zramsay Nov 13, 2018

Choose a reason for hiding this comment

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

the better option (seemingly) was to split the makefile targets - this is how it is in the SDK now

@codecov-io
Copy link

codecov-io commented Nov 13, 2018 •
edited
Loading

Codecov Report

Merging #2824 into develop will decrease coverage by 0.04%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           develop   #2824      +/-   ##
==========================================
- Coverage    62.25%   62.2%   -0.05%     
==========================================
  Files          212     212              
  Lines        17253   17253              
==========================================
- Hits         10740   10732       -8     
- Misses        5610    5616       +6     
- Partials       903     905       +2
Impacted Files Coverage Δ
privval/tcp_server.go 78.57% <0%> (-2.86%) ⬇️
consensus/reactor.go 66.74% <0%> (-0.93%) ⬇️
privval/ipc_server.go 73.58% <0%> (+3.77%) ⬆️

melekes
melekes approved these changes Nov 14, 2018
View reviewed changes
Copy link
Contributor

@melekes melekes left a comment

Choose a reason for hiding this comment

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

🌯

@melekes melekes merged commit bb0e17d into develop Nov 14, 2018
@melekes melekes deleted the zach/arm branch November 14, 2018 10:17
melekes added a commit that referenced this pull request Nov 14, 2018
@melekes
Vagrantfile: install dev_tools
bc5b60a 
Follow-up on #2824
ebuchman
ebuchman reviewed Nov 14, 2018
View reviewed changes
Copy link
Contributor

@ebuchman ebuchman left a comment

Choose a reason for hiding this comment

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

Couldn't we consolidate by making a variable for the go binary and switching based on the OS? seems unnecessary to have a duplicate script ...

scripts/install/install_tendermint_arm.sh
#!/usr/bin/env bash

# XXX: this script is intended to be run from
# a fresh Digital Ocean droplet with Ubuntu
Copy link
Contributor

@ebuchman ebuchman Nov 14, 2018

Choose a reason for hiding this comment

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

Oh?

melekes added a commit that referenced this pull request Nov 15, 2018
@melekes
Vagrantfile: install dev_tools
111d9e4 
Follow-up on #2824
@zramsay
Copy link
Contributor Author

zramsay commented Nov 15, 2018

@ebuchman yeah i was thinking the same thing ... it'll happen

melekes added a commit that referenced this pull request Nov 16, 2018
@melekes
Vagrantfile: install dev_tools
9b4bece 
Follow-up on #2824
ebuchman pushed a commit that referenced this pull request Nov 16, 2018
@melekes @ebuchman
small fixes to spec & http_server & Vagrantfile (#2859)
e6a0d09 
* Vagrantfile: install dev_tools

Follow-up on #2824

* update consensus params spec

* fix test name

* rpc_test: panic if failed to start listener

also
- remove http_server#MustListen
- align StartHTTPServer and StartHTTPAndTLSServer functions

* dep: allow minor releases for grpc
@zramsay
Copy link
Contributor Author

zramsay commented Nov 30, 2018

tendermint/coding#72

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@xla xla Awaiting requested review from xla

3 more reviewers

@ebuchman ebuchman ebuchman left review comments

@liamsi liamsi liamsi left review comments

@melekes melekes melekes approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@zramsay @melekes @codecov-io @ebuchman @liamsi