You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by "maskit (via GitHub)" <gi...@apache.org> on 2023/05/02 17:13:00 UTC
[GitHub] [trafficserver] maskit opened a new pull request, #9670: Don't build native QUIC implementation
maskit opened a new pull request, #9670:
URL: https://github.com/apache/trafficserver/pull/9670
Although we decided to use Quiche and remove the code for ATS's native QUIC implementation, I found that keeping the code on master branch is actually handy and helpful, because I can read/copy the code easily when I implement outgoing QUIC connection with Quiche (the native impl supported outgoing connection).
This PR removes the filenames for the native implementation from Makefiles for now so we don't have to maintain the code, and keeps actual files for reference.
We'll remove the files when we get outgoing QUIC connection work with Quiche.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit merged pull request #9670: Don't build native QUIC implementation
Posted by "maskit (via GitHub)" <gi...@apache.org>.
maskit merged PR #9670:
URL: https://github.com/apache/trafficserver/pull/9670
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] brbzull0 commented on a diff in pull request #9670: Don't build native QUIC implementation
Posted by "brbzull0 (via GitHub)" <gi...@apache.org>.
brbzull0 commented on code in PR #9670:
URL: https://github.com/apache/trafficserver/pull/9670#discussion_r1183482477
##########
configure.ac:
##########
@@ -1451,19 +1427,19 @@ TS_CHECK_BROTLI
TS_CHECK_LUAJIT
# Check for optional quiche library
+enable_quic=no
TS_CHECK_QUICHE
if test "${has_quiche}" = "1"; then
if test "$openssl_is_boringssl" = "1" ; then
enable_quic=yes
-## Doing these again for Quiche
-AM_CONDITIONAL([ENABLE_QUIC], [test "x$enable_quic" = "xyes"])
-TS_ARG_ENABLE_VAR([use], [quic])
-AC_SUBST(use_quic)
-AC_SUBST(has_quiche)
else
AC_MSG_ERROR([Use of BoringSSL is required if Quiche is used.])
fi
fi
+TS_ARG_ENABLE_VAR([use], [quic])
+AC_SUBST(use_quic)
+AC_SUBST(has_quiche)
Review Comment:
`use_quic` or `quiche` both imply the same things from now on, so I wonder if we should keep two separate variables for the QUIC implementation.
No need to change this now anyway, but we can agree and add task for it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] brbzull0 commented on a diff in pull request #9670: Don't build native QUIC implementation
Posted by "brbzull0 (via GitHub)" <gi...@apache.org>.
brbzull0 commented on code in PR #9670:
URL: https://github.com/apache/trafficserver/pull/9670#discussion_r1183481212
##########
configure.ac:
##########
@@ -1451,19 +1427,19 @@ TS_CHECK_BROTLI
TS_CHECK_LUAJIT
# Check for optional quiche library
+enable_quic=no
TS_CHECK_QUICHE
if test "${has_quiche}" = "1"; then
if test "$openssl_is_boringssl" = "1" ; then
enable_quic=yes
-## Doing these again for Quiche
-AM_CONDITIONAL([ENABLE_QUIC], [test "x$enable_quic" = "xyes"])
-TS_ARG_ENABLE_VAR([use], [quic])
-AC_SUBST(use_quic)
-AC_SUBST(has_quiche)
else
AC_MSG_ERROR([Use of BoringSSL is required if Quiche is used.])
fi
fi
+TS_ARG_ENABLE_VAR([use], [quic])
+AC_SUBST(use_quic)
Review Comment:
`use_quic` or `quiche` both imply the same things from now on, so I wonder if we should keep two separate variables for the QUIC implementation.
No need to change this now anyway, but we can agree and add task for it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on a diff in pull request #9670: Don't build native QUIC implementation
Posted by "maskit (via GitHub)" <gi...@apache.org>.
maskit commented on code in PR #9670:
URL: https://github.com/apache/trafficserver/pull/9670#discussion_r1183891114
##########
configure.ac:
##########
@@ -1451,19 +1427,19 @@ TS_CHECK_BROTLI
TS_CHECK_LUAJIT
# Check for optional quiche library
+enable_quic=no
TS_CHECK_QUICHE
if test "${has_quiche}" = "1"; then
if test "$openssl_is_boringssl" = "1" ; then
enable_quic=yes
-## Doing these again for Quiche
-AM_CONDITIONAL([ENABLE_QUIC], [test "x$enable_quic" = "xyes"])
-TS_ARG_ENABLE_VAR([use], [quic])
-AC_SUBST(use_quic)
-AC_SUBST(has_quiche)
else
AC_MSG_ERROR([Use of BoringSSL is required if Quiche is used.])
fi
fi
+TS_ARG_ENABLE_VAR([use], [quic])
+AC_SUBST(use_quic)
+AC_SUBST(has_quiche)
Review Comment:
I thought I can remove it, and I still think we should remove it, but I kept it for now. Because there are files that are used for the both native impl and quiche impl (e.g. iocore/net/QUICNet.cc), and those files `#if TS_HAS_QUICHE `.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org