Skip to content

cmake builds with quiche/quic support #9833

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
Closed
bryancall wants to merge 2 commits into from
Closed

cmake builds with quiche/quic support #9833

bryancall wants to merge 2 commits into from

Conversation

@bryancall
Copy link
Contributor

@bryancall bryancall commented Jun 13, 2023

No description provided.

@bryancall bryancall added the QUIC label Jun 13, 2023
@bryancall bryancall added this to the 10.0.0 milestone Jun 13, 2023
@bryancall bryancall self-assigned this Jun 13, 2023
@bryancall bryancall force-pushed the cmake_quiche branch 2 times, most recently from 30bd92f to 9f405b5 Compare June 14, 2023 15:27
@bryancall bryancall marked this pull request as ready for review June 14, 2023 18:05
@bryancall
Copy link
Contributor Author

bryancall commented Jun 14, 2023

[approve ci]

maskit
maskit reviewed Jun 14, 2023
View reviewed changes
cmake/FindQUICHE.cmake
set(QUIC_LIBRARIES ${QUICHE_LIBRARY} http3 quic)
set(QUIC_INCLUDE_DIRS ${QUICHE_INCLUDE_DIR} ${CMAKE_SOURCE_DIR}/iocore/net/quic)
set(TS_HAS_QUICHE 1)
set(TS_USE_QUIC 1)
Copy link
Member

@maskit maskit Jun 14, 2023

Choose a reason for hiding this comment

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

TS_USE_QUIC becomes 1 if the both Quiche and BoringSSL are available.

enable_quic=no
TS_CHECK_QUICHE
if test "${has_quiche}" = "1"; then
if test "$openssl_is_boringssl" = "1" ; then
enable_quic=yes
else
  AC_MSG_ERROR([Use of BoringSSL is required if Quiche is used.])
fi
fi

JosiahWI
JosiahWI reviewed Jun 14, 2023
View reviewed changes
CMakeLists.txt
Comment on lines +78 to +79
CHECK_INCLUDE_FILE(stdlib.h HAVE_STDLIB_H)
CHECK_INCLUDE_FILE(stdint.h HAVE_STDINT_H)
Copy link
Contributor

@JosiahWI JosiahWI Jun 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
CHECK_INCLUDE_FILE(stdlib.h HAVE_STDLIB_H)
CHECK_INCLUDE_FILE(stdint.h HAVE_STDINT_H)
check_include_file(stdint.h HAVE_STDINT_H)
check_include_file(stdlib.h HAVE_STDLIB_H)

cmake/FindQUICHE.cmake
Comment on lines +20 to +21
find_path(QUICHE_INCLUDE_DIR NAMES quiche.h HINTS ${QUICHE_INSTALL_DIR} PATH_SUFFIXES include)
find_library(QUICHE_LIBRARY NAMES quiche HINTS ${QUICHE_INSTALL_DIR} PATH_SUFFIXES lib)
Copy link
Contributor

@JosiahWI JosiahWI Jun 14, 2023

Choose a reason for hiding this comment

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

Suggested change
find_path(QUICHE_INCLUDE_DIR NAMES quiche.h HINTS ${QUICHE_INSTALL_DIR} PATH_SUFFIXES include)
find_library(QUICHE_LIBRARY NAMES quiche HINTS ${QUICHE_INSTALL_DIR} PATH_SUFFIXES lib)
find_path(QUICHE_INCLUDE_DIR NAMES quiche.h PATHS ${QUICHE_INSTALL_DIR} PATH_SUFFIXES include)
find_library(QUICHE_LIBRARY NAMES quiche PATHS ${QUICHE_INSTALL_DIR} PATH_SUFFIXES lib)

Search the paths specified by the HINTS option. These should be paths computed by system introspection, such as a hint provided by the location of another item already found. Hard-coded guesses should be specified with the PATHS option.
(https://cmake.org/cmake/help/latest/command/find_library.html)

Copy link
Contributor

@JosiahWI JosiahWI Jun 14, 2023

Choose a reason for hiding this comment

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

You don't need PATH_SUFFIXES here at all. lib/ and include/ are already default search paths.

cmake/FindQUICHE.cmake
find_path(QUICHE_INCLUDE_DIR NAMES quiche.h HINTS ${QUICHE_INSTALL_DIR} PATH_SUFFIXES include)
find_library(QUICHE_LIBRARY NAMES quiche HINTS ${QUICHE_INSTALL_DIR} PATH_SUFFIXES lib)

INCLUDE(FindPackageHandleStandardArgs)
Copy link
Contributor

@JosiahWI JosiahWI Jun 14, 2023

Choose a reason for hiding this comment

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

Suggested change
INCLUDE(FindPackageHandleStandardArgs)
include(FindPackageHandleStandardArgs)

iocore/net/quic/CMakeLists.txt
Comment on lines +36 to +38
${CMAKE_SOURCE_DIR}/iocore/net
${CMAKE_SOURCE_DIR}/iocore/net/quic
${CMAKE_SOURCE_DIR}/iocore/eventsystem
Copy link
Contributor

@JosiahWI JosiahWI Jun 14, 2023

Choose a reason for hiding this comment

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

Suggested change
${CMAKE_SOURCE_DIR}/iocore/net
${CMAKE_SOURCE_DIR}/iocore/net/quic
${CMAKE_SOURCE_DIR}/iocore/eventsystem
"${CMAKE_SOURCE_DIR}/iocore/net"
"${CMAKE_SOURCE_DIR}/iocore/net/quic"
"${CMAKE_SOURCE_DIR}/iocore/eventsystem"

proxy/http3/CMakeLists.txt
Comment on lines +40 to +43
${CMAKE_SOURCE_DIR}/iocore/net
${CMAKE_SOURCE_DIR}/iocore/net/quic
${CMAKE_SOURCE_DIR}/iocore/eventsystem
${CMAKE_SOURCE_DIR}/mgmt
Copy link
Contributor

@JosiahWI JosiahWI Jun 14, 2023

Choose a reason for hiding this comment

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

Suggested change
${CMAKE_SOURCE_DIR}/iocore/net
${CMAKE_SOURCE_DIR}/iocore/net/quic
${CMAKE_SOURCE_DIR}/iocore/eventsystem
${CMAKE_SOURCE_DIR}/mgmt
"${CMAKE_SOURCE_DIR}/iocore/net"
"${CMAKE_SOURCE_DIR}/iocore/net/quic"
"${CMAKE_SOURCE_DIR}/iocore/eventsystem"
"${CMAKE_SOURCE_DIR}/mgmt"

cmake/FindQUICHE.cmake
Comment on lines +27 to +30
set(QUICHE_LIBRARY_DIR ${QUICHE_INSTALL_DIR}/lib)
set(QUIC_LIBRARY_DIRS ${QUICHE_LIBRARY_DIR})
set(QUIC_LIBRARIES ${QUICHE_LIBRARY} http3 quic)
set(QUIC_INCLUDE_DIRS ${QUICHE_INCLUDE_DIR} ${CMAKE_SOURCE_DIR}/iocore/net/quic)
Copy link
Contributor

@JosiahWI JosiahWI Jun 14, 2023

Choose a reason for hiding this comment

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

Suggested change
set(QUICHE_LIBRARY_DIR ${QUICHE_INSTALL_DIR}/lib)
set(QUIC_LIBRARY_DIRS ${QUICHE_LIBRARY_DIR})
set(QUIC_LIBRARIES ${QUICHE_LIBRARY} http3 quic)
set(QUIC_INCLUDE_DIRS ${QUICHE_INCLUDE_DIR} ${CMAKE_SOURCE_DIR}/iocore/net/quic)
set(QUICHE_LIBRARY_DIR "${QUICHE_INSTALL_DIR}/lib")
set(QUIC_LIBRARY_DIRS "${QUICHE_LIBRARY_DIR}")
set(QUIC_LIBRARIES "${QUICHE_LIBRARY}" http3 quic)
set(QUIC_INCLUDE_DIRS "${QUICHE_INCLUDE_DIR} ${CMAKE_SOURCE_DIR}/iocore/net/quic")

@JosiahWI
Copy link
Contributor

JosiahWI commented Jun 14, 2023

Indentation seems very inconsistent. I think we're using 4-space indents in all the CMake stuff - CMake itself uses 2-space indents, which I like, but it might be good to have a unified style.

JosiahWI
JosiahWI reviewed Jun 14, 2023
View reviewed changes
cmake/FindQUICHE.cmake
find_library(QUICHE_LIBRARY NAMES quiche HINTS ${QUICHE_INSTALL_DIR} PATH_SUFFIXES lib)

INCLUDE(FindPackageHandleStandardArgs)
FIND_PACKAGE_HANDLE_STANDARD_ARGS(QUICHE DEFAULT_MSG QUICHE_LIBRARY QUICHE_INCLUDE_DIR)
Copy link
Contributor

@JosiahWI JosiahWI Jun 14, 2023

Choose a reason for hiding this comment

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

Suggested change
FIND_PACKAGE_HANDLE_STANDARD_ARGS(QUICHE DEFAULT_MSG QUICHE_LIBRARY QUICHE_INCLUDE_DIR)
find_package_handle_standard_args(QUICHE DEFAULT_MSG QUICHE_LIBRARY QUICHE_INCLUDE_DIR)

@JosiahWI JosiahWI mentioned this pull request Jun 21, 2023
Merged
@bryancall
Copy link
Contributor Author

bryancall commented Jun 26, 2023

#9892 superseded this PR

@bryancall bryancall closed this Jun 26, 2023
@bryancall bryancall removed this from the 10.0.0 milestone Jun 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@maskit maskit maskit left review comments

@JosiahWI JosiahWI JosiahWI left review comments

Assignees

@bryancall bryancall

Labels

QUIC

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@bryancall @JosiahWI @maskit