-
Notifications
You must be signed in to change notification settings - Fork 844
tools: H3 tools build script. Add go+boringssl #9606
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
tools: H3 tools build script. Add go+boringssl #9606
Conversation
bneradt
left a comment
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.
Looks great to me. Looks very similar to our Dockerfiles for this, which is good:
7718f66 to
8359562
Compare
|
A related change: |
8359562 to
ae6bdc8
Compare
bneradt
left a comment
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.
Looks good generally. Just one observation.
ae6bdc8 to
ef21134
Compare
bneradt
left a comment
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.
One other thing comes to mind: we should update the yum install and apt install lines. I think this will need cargo and rust added, right?
for reference, cargo and rust are also mentioned here among others that aren't mentioned here which may also be needed. |
|
What is the intent of this? I see it building boringssl but not building anything that uses it. Are you planning on running this script to install deps for building ATS? I'd prefer this script not automatically install a new version of go in /usr/local. Can it check for an existing go installation beforehand? |
cmcfarlen
left a comment
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.
I'd prefer this script not automatically install a new version of go in /usr/local. Can it check for an existing go installation beforehand?
This is to build the toolchain around H3/QUIC and QUICHE, boringssl is now mandatory to build ATS with quiche. |
I don't use go so please let me know if this is an issue, as I do not use go it makes no diff to me where it sits. But happy to adjust it if we all agreed. Also -> https://github.com/apache/trafficserver/pull/9606/files#r1165525319 |
|
For me I'd prefer this script use the installed go if adequate and if this script installs go that it not go in any directory that might end up being in the user's PATH. |
boringssl should be done before quiche. quiche should use the boringssl folder to link.
a8aae90 to
b4c9c7e
Compare
| [ ! -d quiche ] && git clone --recursive https://github.com/cloudflare/quiche.git | ||
| cd quiche | ||
| cargo build -j4 --package quiche --release --features ffi,pkg-config-meta,qlog | ||
| QUICHE_BSSL_PATH=${BASE}/boringssl QUICHE_BSSL_LINK_KIND=dylib cargo build -j4 --package quiche --release --features ffi,pkg-config-meta,qlog |
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.
we use the libs from the boringssl we just built.
bneradt
left a comment
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.
Looks good. Just a few suggestions.
|
I tested this with my suggested changes for updating the CI builds, and the builds worked fine. I'll be updating the images shortly (either tonight or tomorrow). Thank you for working on this @brbzull0 . |
|
[approve ci autest rocky] |
|
Looks like the connect, polite_hook_wait, h2enable, h2enable_no_accept_threads, and proxy_protocol tests are now failing with the updated tools. I'll look into this shortly. On the plus side, while the quiche tests are still running and there are failures, at least the active_timeout test passes now: https://ci.trafficserver.apache.org/view/10.x-master/job/master/job/quiche/34/console |
|
I see...the curl in the updated tools is very new, 7.88. We'll have to update some gold files I believe. I can work on that in a bit. |
I updated the gold file tests for the new version of curl here: |
|
[approve ci autest] |
great! the other one that was failing should pass once we get #9642 in. Thanks for helping with this Brian!! |
|
quiche branch autests are passing with these changes integrated into the rockylinux docker image: Thanks for working on this. It's nice to see that build green. |
|
[approve ci autest] |
bneradt
left a comment
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.
Looks great to me, @brbzull0 .
No description provided.