-
Notifications
You must be signed in to change notification settings - Fork 78
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
Size optimization ngtcp2 and change to use OpenSSL3/oqsprovider #195
Size optimization ngtcp2 and change to use OpenSSL3/oqsprovider #195
Conversation
Understood. Have a great vacation! |
b38b68a
to
4bbb57b
Compare
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.
Apologies for the long silence on our side :-( and Thanks very much again for this PR -- particularly for the update to OpenSSL3/removing the dependency on oqs-openssl111!
In general, I'm happy with this PR, but when running it locally, the build failed, so CI will also fail :-( So, can I ask you to re-run the build along the lines of what the CI logic does and update the code as necessary? I'm particularly concerned about the hard-coded library version copies (see single comment ). For the actual commands that need to succeed for CI see
oqs-demos/.circleci/config.yml
Lines 406 to 414 in 9a9b35f
docker network create ngtcp2-test | |
docker run --network ngtcp2-test --name oqs-ngtcp2server oqs-ngtcp2-server bash -c 'server --groups kyber512 "*" 6000 CA.key CA.crt' & | |
docker run --network ngtcp2-test -it --name oqs-ngtcp2client oqs-ngtcp2-client bash -c 'client --exit-on-first-stream-close --groups kyber512 oqs-ngtcp2server 6000' | |
docker logs oqs-ngtcp2client | grep "QUIC handshake has been confirmed" | |
docker rm oqs-ngtcp2client | |
docker stop oqs-ngtcp2server | |
docker rm oqs-ngtcp2server | |
docker network rm ngtcp2-test | |
working_directory: ngtcp2 |
It turns out the build was failing due to changes in the library versions. It builds properly now that I have addressed this by using wildcards. Once again thank you for your valuable input! |
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.
It turns out the build was failing due to changes in the library versions. It builds properly now that I have addressed this by using wildcards. Once again thank you for your valuable input!
Thank you for the update! Everything now indeed builds OK. However, the CI test (still) fails (see command sequence above): For one, the new docker image doesn't contain bash, but secondly, no (test) CA is "baked into" the image: How would you recommend to change things to have a good CI test for this PR?
@Keelan10 I added your PR code to our CI chain and various failures ensued: If you still would like to see this PR move forward, would you please re-base your code such as to get past the non-PR related CI failures (of course with the goal to see that your new code also passes CI)? |
Refactored Dockerfiles to use multi-stage build for both the client and server.
* Updated documentation * Replaced absolute links with relative links
3e520e4
to
e64d4d3
Compare
Hello, |
Superseded/replaced by #211 |
Hi,
I have refactored the Dockerfiles for ngtcp2 to use multi-stage build. This reduces the size of each docker image to ∼32 MB.
Thank you for reviewing!
Edit: ngtcp2 has been moved to openssl3.