Added go modules for reproducible build (known dep versions). #24
Conversation
bwplotka
force-pushed
the
modules
branch
2 times, most recently
from
bc622ce to
6d3b861
Compare
Makefile
Outdated
| @@ -1,4 +1,5 @@ | |||
| GO := go | |||
| STATICCHECK ?= staticcheck | |||
There was a problem hiding this comment.
Shouldn't this be
| STATICCHECK ?= staticcheck | |
| STATICCHECK ?= get_staticcheck |
?
I'm also not clear what the purpose of this change is. It appears to make the target name configurable but not much else. (Obviously not a make guru over here.)
There was a problem hiding this comment.
So, in theory this change is to actually put a binary itself as non phony "command". This allows make to decide when to install it (only if the file is not there).
There was a problem hiding this comment.
Sorry, I still don't get that.
What I'm seeing is:
- Two
staticchecktargets that depend on each other when theSTATICCHECKenvironment variable is not set; and - Some arbitrarily named target (instead of a fixed
get_staticcheck), possibly conflicting with other targets when theSTATICCHECKenvironment variable is set to some value.
There was a problem hiding this comment.
You are right. Also it was not working as you need full path to staticcheck to make it work.
You also need $GOBIN set to have any go install be meaningful (otherwise it does not know where to install), so I hope this requirement makes sense to you.
|
BTW how do you find |
bwplotka
force-pushed
the
modules
branch
2 times, most recently
from
da8bafc to
7042c0d
Compare
There was a problem hiding this comment.
Approved. I'm curious if you can merge this or whether I have to do it.
BTW how do you find
staticcheck?
To be honest, I most likely copied the Makefile from some place or other, it wasn't a conscious choice. But IIRC, it did point out some actual issues with my code in the past. If you have a more informed opinion, I'm happy to hear it.
| @@ -1,4 +1,5 @@ | |||
| GO := go | |||
| STATICCHECK := ${GOBIN}/staticcheck | |||
There was a problem hiding this comment.
FYI, GOBIN on my machine is "". Should it be something like "${GOPATH}/bin"?
There was a problem hiding this comment.
So the problem is that GOPATH is not really needed now AFAIK
There was a problem hiding this comment.
but we want definitive place to store binaries so.. defining GOBIN for it makes sense I guess?
|
Oh, and FYI the Travis CI build failed. AFAICT something to do with modules. |
bwplotka
force-pushed
the
modules
branch
2 times, most recently
from
a2b8d39 to
ac973d8
Compare
Signed-off-by: Bartek Plotka <bwplotka@gmail.com>
|
This was more painful then I thought and thanks for detailed review! Should be all good now @free |
|
I gather that GitHub didn't allow you to merge the PR yourself. Is that right? |
No vendoring though (as hosting someone's code is not really a correct path).
Signed-off-by: Bartek Plotka bwplotka@gmail.com