- Notifications
You must be signed in to change notification settings - Fork927
feat: add boringcrypto builds for linux#9528
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Signed-off-by: Spike Curtis <spike@coder.com>
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Do we need tobuild with boringcrypto, or just provide theoption to build with boringcrypto? It's a shame that we need to leave the debugging symbols in.
Uh oh!
There was an error while loading.Please reload this page.
Re: debug symbols, could we strip them after we’ve verified the build? |
We could, but then customers wouldn't be able to verify. |
We need to be able to ship boringcrypto builds, and I don't think we should ship variants for a single OS/arch: i.e. if you run on Linux amd64 you get boringcrypto. |
I was thinking we would add We could also archive the non-stripped binaries for potential verification and/or use where symbols maybe be needed. |
mafredri commentedSep 5, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Actually, on second thought, we could utilize build tags here: //go:build boringcryptopackage buildinfoimport"crypto/boring"var (boringcrypto=boring.Enabled()) With this, I assume we may not even need to verify via symbols 🤔 (but we could, just to be safe). |
Signed-off-by: Spike Curtis <spike@coder.com>
Thanks@mafredri --- added to BuildInfo |
mafredri left a comment• edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Looking nice!
Edit: Looks likeTestVersion/JSON_output needs updating.
| goexp="boringcrypto" | ||
| fi | ||
| GOEXPERIMENT="$goexp" CGO_ENABLED="$cgo" GOOS="$os" GOARCH="$arch" GOARM="$arm_version" go build \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Does-boringcrypto also need to be added togo build args?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
nope, I testedcoder version on a binary I built.GOEXPERIMENT=boringcrypto is enough.
Signed-off-by: Spike Curtis <spike@coder.com>
Uh oh!
There was an error while loading.Please reload this page.
fixes#9087
Adds support for building with Boring Crypto on Linux, when not cross-compiling.
It's maybe a little surprising for us to build different versions of Coder binaries depending on where you build. I chose to do it this way so that everyone gets boring crypto if available on their platform, so that we're not having people develop against non-boring-crypto and then shipping a different build that we haven't been testing with.
It also leaves debug symbols in boring crypto binaries, since it is hard to verify we are building with boring crypto without these.This increases binary sizes by about 5MiB for the linux amd64 slim binary (39 -> 44) and 8MiB (184 -> 192) for the fat linux amd64 binary.