Conversation
aerth
commented
Oct 8, 2018
aerth
commented
- Fixes build on FreeBSD
- simplifies install.sh
- removes bash dependency (from this package at least)
aerth
force-pushed
the
imports
branch
4 times, most recently
from
da3b8e5 to
5050845
Compare
|
Thanks for the PR. I think the Travis failure is safe to ignore; it's due to upstream gometalinter recently adding a new linter that we haven't yet dealt with. Just to be safe, I'll hold off on merging this until a fix for the Travis fix is merged in case there's another issue Travis found that I missed while skimming the output. |
| NEW_PACKAGE="${NEW_PACKAGE//\//\\/}" | ||
| sed -i "s/${OLD_PACKAGE}/${NEW_PACKAGE}/g" ./*.go ${GOPATH}/src/vendor/golang.org/x/crypto/cryptobyte/*.go | ||
| #!/bin/sh | ||
| set -e |
There was a problem hiding this comment.
Am I correct in inferring that the set -u, set -o pipefail, and shopt -s failglob were removed for compatibility with sh? I'm honestly not sure how I feel about this change; those options are useful from a safety perspective. @hlandau what do you think about this?
There was a problem hiding this comment.
Yes and, well, theres no piping going on so theres no need for pipefail
x509/install.sh
Outdated
| sed -i "s/${OLD_PACKAGE}/${NEW_PACKAGE}/g" ./*.go ${GOPATH}/src/vendor/golang.org/x/crypto/cryptobyte/*.go | ||
| #!/bin/sh | ||
| set -e | ||
| gopath="$(go env GOROOT)" |
There was a problem hiding this comment.
I prefer that this variable be called goroot rather than gopath, since gopath means something else in Go.
There was a problem hiding this comment.
was leftover from testing, thanks for pointing that out!
x509/install.sh
Outdated
| test -d $gopath | ||
| test -d $x509path | ||
| cp -av $x509path/testdata . | ||
| cp -v $x509path/*.* . |
There was a problem hiding this comment.
Why leave out the -a here? And why two cp commands here? Is the single cp in the original broken in some way?
| cp -av $x509path/testdata . | ||
| cp -v $x509path/*.* . | ||
| rm x509_test.go | ||
| sed -i.bak 's/golang_org/golang.org/g' x509.go |
There was a problem hiding this comment.
It looks like you've removed the copying of the cryptobyte package... is that copy actually unnecessary now? I seem to remember it being required when I was porting to Go 1.10.x, is that no longer the case?
There was a problem hiding this comment.
no longer necessary it seems (go 1.11.1 here)
|
The |
2 participants
