feat: getsolc binary to locate or download Solidity compiler
#1244
Conversation
| return fmt.Errorf("unsupported OS %q", goos) | ||
| } | ||
|
|
||
| // solc only provides amd64 binaries, but this can be ignored if there is a |
There was a problem hiding this comment.
(No action required) I guess rosetta is fine, but what is up with automated release procedures not being updated to create arm64 binaries? I guess it's an indication that most people are using a package manager that does ship arm64.
| } | ||
| defer jsonList.Body.Close() | ||
| var list solcList | ||
| if err := json.NewDecoder(jsonList.Body).Decode(&list); err != nil { |
There was a problem hiding this comment.
(No action required) Is there an advantage to using Decode vs Unmarshal where the data is in-memory (as I'm assuming is the case here)?
| return "", false | ||
| } | ||
|
|
||
| which := exec.CommandContext(ctx, "which", "solc") |
There was a problem hiding this comment.
(No action required) Maybe prefer command as per https://www.shellcheck.net/wiki/SC2230? It is optional and a bit pedantic, but its been enforced on me before.
I guess a slight con of using golang over bash for scripting is that we don't benefit from a scripting-centric linter? Not a deal-breaker by any means, but we'll inevitably have shell interaction in golang scripts and in the absence of a linter we'll have to rely on review.
| // succesful location of a matching binary. | ||
| func (c *config) bestEffortFindInPATH(ctx context.Context) (string, bool) { | ||
| solc := exec.CommandContext(ctx, "solc", "--version") | ||
| out, err := solc.CombinedOutput() |
There was a problem hiding this comment.
(No action required) For general-purpose scripting in golang, maybe we'll want a standard way to invoke commands so that we can replicate the behavior of set -x for the whole script? Or maybe I'm just scarred from too much bash and should get used to the idea of using a debugger instead... 😄
| return err | ||
| } | ||
|
|
||
| fmt.Fprintf(os.Stderr, "Keccak256 of download: %#x\n", hash.Sum(nil)) |
There was a problem hiding this comment.
(No action required) Best practice for the use of stderr vs stdout? I've defaulted to thinkiing of stderr as for error conditions - which isn't the case here - but I recall that a more nuanced view is prevalent among experts.
| flag.Parse() | ||
|
|
||
| if err := c.run(context.Background()); err != nil { | ||
| fmt.Fprintln(os.Stderr, err) |
There was a problem hiding this comment.
(No action required) Thoughts on using panic in a script to provide immediate feedback and a stack trace? I've heard it suggested that getting a stack trace in the context of a script can be preferential to passing errors around, but its use would probably have to be limited to main entrypoints or reusability of library functionality would be compromised.
Why this should be merged
How this works
How this was tested
How is this documented
TODO before review
list.jsonfiles instead of downloading them (trusted hashes)