Skip to content

autoconf: Add lib flags for the quiche build test. #9679

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.

Sign up for GitHub

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

Jump to bottom
Merged
bneradt merged 2 commits into from
May 15, 2023
Merged

autoconf: Add lib flags for the quiche build test. #9679

bneradt merged 2 commits into from
May 15, 2023

Conversation

@brbzull0
Copy link
Contributor

@brbzull0 brbzull0 commented May 4, 2023 •
edited
Loading

More likely we need this to get the test build passing and have quiche properly detected.

@brbzull0 brbzull0 self-assigned this May 4, 2023
@brbzull0 brbzull0 added the QUIC label May 4, 2023
@maskit
Copy link
Member

maskit commented May 4, 2023

I faced the issue on one of my boxes, and forgot to fix it. I think I did the same thing at that time and it worked, but don't we have two -lssl -lcrypto as the final LIBS output (i.e. ones from quiche.m4 and ones from crypto.m4)?

@brbzull0
Copy link
Contributor Author

brbzull0 commented May 4, 2023

I faced the issue on one of my boxes, and forgot to fix it. I think I did the same thing at that time and it worked, but don't we have two -lssl -lcrypto as the final LIBS output (i.e. ones from quiche.m4 and ones from crypto.m4)?

That I don't know, with this we end up with a line like this:
config.log

configure:23648: checking for quiche_connect in -lquiche
configure:23673: cc -o conftest  -D_GNU_SOURCE -I/opt/boringssl/include -DOPENSSL_NO_SSL_INTERN -I/opt/quiche/include -L/opt/boringssl/lib -L/opt/quiche/lib conftest.c -lquiche  -lpthread -ldl  -lquiche -lssl -lcrypto >&5

so -lquiche -lpthread -ldl -lquiche -lssl -lcrypto, I can probably remove the first quiche flag from quiche_test_libs which will be added after anyway.

... conftest.c -lquiche  -lpthread -ldl  -lssl -lcrypto

@bneradt
Copy link
Contributor

bneradt commented May 4, 2023

[approve ci cmake]

@maskit
Copy link
Member

maskit commented May 4, 2023

Not a big deal, but with this change I see this (look at the last part).

/bin/sh ../libtool --tag=CXX --mode=link ccache c++ -std=c++17 -g -pipe -Wall -Qunused-arguments -Wextra -Wno-unused-parameter -Werror -Wno-invalid-offsetof -fno-omit-frame-pointer -fsanitize=address -R/Users/mkitajo/opt/boringssl/lib -R/opt/homebrew/Cellar/pcre/8.45/lib -R/Users/mkitajo/opt/quiche/lib -L/Users/mkitajo/src/github.com/trafficserver/lib/swoc -L/Users/mkitajo/opt/boringssl/lib -L/opt/homebrew/Cellar/pcre/8.45/lib -L/Users/mkitajo/opt/quiche/lib -o traffic_cache_tool/traffic_cache_tool traffic_cache_tool/traffic_cache_tool-CacheDefs.o traffic_cache_tool/traffic_cache_tool-CacheTool.o traffic_cache_tool/traffic_cache_tool-CacheScan.o ../src/tscore/.libs/ArgParser.o ../src/tscore/.libs/ink_assert.o ../src/tscore/.libs/ink_error.o ../src/tscore/.libs/ink_file.o ../src/tscore/.libs/ink_memory.o ../src/tscore/.libs/ink_mutex.o ../src/tscore/.libs/ink_string.o ../src/tscore/.libs/BufferWriterFormat.o ../src/tscore/.libs/InkErrno.o ../src/tscore/.libs/Errata.o ../src/tscpp/util/.libs/TextView.o ../src/tscore/.libs/Regex.o ../src/tscore/.libs/CryptoHash.o ../src/tscore/.libs/MMH.o ../src/tscore/.libs/Version.o ../src/tscore/.libs/Regression.o ../src/tscore/.libs/ink_args.o ../src/tscore/.libs/ParseRules.o ../src/tscore/.libs/SourceLocation.o -ltsswoc -lssl -lcrypto -lpcre -lquiche -lssl -lcrypto

@brbzull0 brbzull0 force-pushed the quiche_autoconf_add_link_flags branch from 618c33d to c941e7e Compare May 5, 2023 11:42
@brbzull0 brbzull0 force-pushed the quiche_autoconf_add_link_flags branch from c941e7e to 740f56f Compare May 5, 2023 11:49
@brbzull0
Copy link
Contributor Author

brbzull0 commented May 5, 2023

Not a big deal, but with this change I see this (look at the last part).

/bin/sh ../libtool --tag=CXX --mode=link ccache c++ -std=c++17 -g -pipe -Wall -Qunused-arguments -Wextra -Wno-unused-parameter -Werror -Wno-invalid-offsetof -fno-omit-frame-pointer -fsanitize=address -R/Users/mkitajo/opt/boringssl/lib -R/opt/homebrew/Cellar/pcre/8.45/lib -R/Users/mkitajo/opt/quiche/lib -L/Users/mkitajo/src/github.com/trafficserver/lib/swoc -L/Users/mkitajo/opt/boringssl/lib -L/opt/homebrew/Cellar/pcre/8.45/lib -L/Users/mkitajo/opt/quiche/lib -o traffic_cache_tool/traffic_cache_tool traffic_cache_tool/traffic_cache_tool-CacheDefs.o traffic_cache_tool/traffic_cache_tool-CacheTool.o traffic_cache_tool/traffic_cache_tool-CacheScan.o ../src/tscore/.libs/ArgParser.o ../src/tscore/.libs/ink_assert.o ../src/tscore/.libs/ink_error.o ../src/tscore/.libs/ink_file.o ../src/tscore/.libs/ink_memory.o ../src/tscore/.libs/ink_mutex.o ../src/tscore/.libs/ink_string.o ../src/tscore/.libs/BufferWriterFormat.o ../src/tscore/.libs/InkErrno.o ../src/tscore/.libs/Errata.o ../src/tscpp/util/.libs/TextView.o ../src/tscore/.libs/Regex.o ../src/tscore/.libs/CryptoHash.o ../src/tscore/.libs/MMH.o ../src/tscore/.libs/Version.o ../src/tscore/.libs/Regression.o ../src/tscore/.libs/ink_args.o ../src/tscore/.libs/ParseRules.o ../src/tscore/.libs/SourceLocation.o -ltsswoc -lssl -lcrypto -lpcre -lquiche -lssl -lcrypto

I fixed this. I was wrong adding the flags to LIBS which will be used everywhere. Now should be fine.

Docs always help :P

AC_CHECK_LIB (library, function, [action-if-found], [action-if-not-found], [other-libraries])
... 
The other-libraries argument should be limited to cases where it is desirable to
test for one library in the presence of another that is not already in LIBS.

Better not pullulate the LIBS as we also have QUICHE_LIB and OPENSSL_LIBS for this needs.

@brbzull0 brbzull0 added this to the 10.0.0 milestone May 5, 2023
@brbzull0
Copy link
Contributor Author

brbzull0 commented May 5, 2023

[approve ci rocky]

@brbzull0 brbzull0 force-pushed the quiche_autoconf_add_link_flags branch from 50616bd to 868754c Compare May 5, 2023 18:14
@brbzull0 brbzull0 force-pushed the quiche_autoconf_add_link_flags branch from 868754c to 20cbc7e Compare May 5, 2023 19:10
@brbzull0 brbzull0 marked this pull request as ready for review May 8, 2023 11:10
@bryancall bryancall requested a review from maskit May 8, 2023 22:11
@brbzull0
Copy link
Contributor Author

brbzull0 commented May 10, 2023 •
edited
Loading

I'll add an update here.

To get this working and have libquiche.so linked with the right Borinssl library you have to ad the bss lib path to the LD_LIBRARY_PATH as it seems quiche is not including the rpath into the lib info.
Trying to figure this out and make some fixes somewhere.

created a pr for quiche build to include the passed boringssl into the rpath

@brbzull0 brbzull0 changed the title autoconf: Add lib flags for the quiche build test autoconf: Add lib flags for the quiche build test. May 10, 2023
bneradt pushed a commit to bneradt/trafficserver-ci that referenced this pull request May 15, 2023
Update quiche builds against boringssl
fef1c30 
Pulls in Damian's fixes to build_h3_tools.sh from Damian's:
apache/trafficserver#9679
@apache apache deleted a comment from brbzull0 May 15, 2023
@bneradt bneradt mentioned this pull request May 15, 2023
Merged
bneradt added a commit to apache/trafficserver-ci that referenced this pull request May 15, 2023
@bneradt
Update quiche builds against boringssl (#164)
0f3758f 
Pulls in Damian's fixes to build_h3_tools.sh from Damian's:
apache/trafficserver#9679
bneradt
bneradt reviewed May 15, 2023
View reviewed changes
Copy link
Contributor

@bneradt bneradt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. I'll let another non-Yahoo person approve.

I applied the build_h3_tools.sh changes to our CI's rockylinux:8 and fedoar:38 docker images and those are working fine, so that's a good confirmation of these changes.

@bneradt bneradt merged commit 2b0a4c5 into apache:master May 15, 2023
@bneradt bneradt mentioned this pull request Jun 6, 2023
Closed
cmcfarlen pushed a commit to cmcfarlen/trafficserver that referenced this pull request Jun 3, 2024
@zwoop
Merge remote-tracking branch 'asf/master'
e3b86ae 
* asf/master:
  Fix ttmsh log field (apache#9722)
  Add CentOS to the required builds (apache#9721)
  Fix H3 transaction leak (apache#9714)
  Only need to include eventfd for native mode. This was using the wrong define anyway (apache#9711)
  Cleanup: remove ts::Buffer from LogField. (apache#9665)
  Cleanup: remove ts::Buffer from URL.cc (apache#9663)
  Check the calling thread of Ethread::schedule_local (apache#9691)
  build_h3_tools.sh: Remove an unneeded dir check (apache#9710)
  autoconf: Add lib flags for the quiche build test. (apache#9679)
  Remove deprecated debug output functions from 13 source files. (apache#9676)
  Changes for C++23 (apache#9703)
  QUIC: Add a unit tests to validate that the qlog file is generated (and no crashes) (apache#9668)
  libswoc: Update to 1.4.10 (apache#9700)
  Reload hosting.config on TASK thread (apache#9699)
  Changes for C++20 (apache#9701)
  Make io_uring or thread AIO modes a startup time decision (vs compile time) (apache#9630)
  Replace curl with proxy verifier in proxy protocol tests (apache#9684)
  Fix event queue corruption on PreWarmManager::reconfigure (apache#9692)
  Fixes crashes around OCSP with FetchSM (apache#9672)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bneradt bneradt bneradt left review comments

@maskit maskit maskit approved these changes

Assignees

@brbzull0 brbzull0

Labels

QUIC

Projects

None yet

Milestone

10.0.0

Development

Successfully merging this pull request may close these issues.

3 participants

@brbzull0 @maskit @bneradt