chore(Makefile): add gopath validation and targets to run nodes for a local dev network#3161
chore(Makefile): add gopath validation and targets to run nodes for a local dev network#3161kanishkatn wants to merge 4 commits intodevelopmentfrom
Conversation
kanishkatn
requested review from
EclesioMeloJunior,
edwardmack,
jimjbrettj,
kishansagathiya,
noot,
qdm12 and
timwu20
as code owners
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## development #3161 +/- ##
===============================================
- Coverage 51.72% 51.63% -0.09%
===============================================
Files 220 220
Lines 28285 28285
===============================================
- Hits 14631 14606 -25
- Misses 12332 12362 +30
+ Partials 1322 1317 -5 |
kanishkatn
changed the title
| ifndef GOPATH | ||
| $(error GOPATH is not set) | ||
| endif |
There was a problem hiding this comment.
Why is this check needed?
I think documentation precises you need Go installed, and that should set GOPATH right?
As in, if we have this check, we would open the door to add a lot more checks such as, do you have go installed, the right go version, do you have g++ installed for wasmer etc.
There was a problem hiding this comment.
hmm, I tried running it on a new laptop which didn't have the GOPATH set. I hadn't used it for any other Go projects yet.
The error that was thrown didn't say anything about the GOPATH, so thought of adding the check in.
There was a problem hiding this comment.
I think I agree with @qdm12 here, this feels a bit much for me. Im not sure it's within the scope of our code to make sure people set up their go env correctly
There was a problem hiding this comment.
See https://go.dev/doc/gopath_code
The GOPATH environment variable specifies the location of your workspace. It defaults to a directory named go inside your home directory, so $HOME/go on Unix, $home/go on Plan 9, and %USERPROFILE%\go (usually C:\Users\YourName\go) on Windows.
Let's remove it, I think you just had a fluke on your system and GOPATH didn't get set. In all my systems it's set automagically
| ifndef GOPATH | ||
| $(error GOPATH is not set) | ||
| endif |
There was a problem hiding this comment.
Why is this check needed?
I think documentation precises you need Go installed, and that should set GOPATH right?
As in, if we have this check, we would open the door to add a lot more checks such as, do you have go installed, the right go version, do you have g++ installed for wasmer etc.
| ``` | ||
| make init-alice | ||
| ``` | ||
|
|
||
| ``` | ||
| make init-bob | ||
| ``` | ||
|
|
||
| ``` | ||
| make init-charlie | ||
| ``` |
There was a problem hiding this comment.
nit maybe merge these in a single code block?
| ``` | ||
| make run-alice | ||
| ``` | ||
|
|
||
| ``` | ||
| make run-bob | ||
| ``` | ||
|
|
||
| ``` | ||
| make run-charlie | ||
| ``` |
There was a problem hiding this comment.
nit maybe merge these in a single code block?
| ``` | ||
| make start-alice | ||
| ``` | ||
|
|
||
| ``` | ||
| make start-bob | ||
| ``` | ||
|
|
||
| ``` | ||
| make start-charlie | ||
| ``` |
There was a problem hiding this comment.
nit maybe merge these in a single code block?
jimjbrettj
left a comment
jimjbrettj
left a comment
There was a problem hiding this comment.
Looks good, but some unresolved comments around checking GOPATH
|
Closing this as the make targets aren't really helpful in the new cli #3173 |
5 participants
Changes
installtarget to make sure GOPATH is setPrimary Reviewer
@timwu20