macOS Support #1197
Open
macOS Support #1197
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
See:
Building on macOS.txt
macOS Implementation.txt
The following lists changes in somewhat chronological order, most
recent first.
---------------------------------------------
Ran "clang-format -i" on the following files:
include/pistache/client.h
include/pistache/mailbox.h
src/common/os.cc
src/common/string_logger.cc
subprojects/cpp-httplib/httplib.h
tests/cookie_test_3.cc
tests/helpers/fd_utils.cc
tests/helpers_test.cc
tests/http_client_test.cc
tests/http_server_test.cc
tests/https_server_test.cc
tests/listener_test.cc
tests/listener_tls_test.cc
tests/reactor_test.cc
tests/request_size_test.cc
tests/streaming_test.cc
tests/tcp_client.h
tests/threadname_test.cc
---------------------------------------------
Added additional debug logging to listener_test.cc
We had the following error, after many repeat test runs:
pistache / listener_test
[ RUN ] CloseOnExecTest.socket_leaked
unknown file: Failure
C++ exception with description "Address already in use" thrown in
the test body.
[ FAILED ] CloseOnExecTest.socket_leaked (5 ms)
listener_test.cc has a function "get_free_port()" which comments:
// This is just done to get the value of a free port. The socket
// will be closed after the closing curly bracket and the port
// will be free again (SO_REUSEADDR option). In theory, it is
// possible that some application grab this port before we bind
// it again...
I suspect that that's what happened to cause test to fail.
tests/listener_test.cc | 55 ++++++++++++++++++++++++++++++++++++++++++++------
1 file changed, 49 insertions(+), 6 deletions(-)
---------------------------------------------
Mention macOS in README
Update version
README.md | 5 +++++
version.txt | 2 +-
2 files changed, 6 insertions(+), 1 deletion(-)
---------------------------------------------
Doing a guard of queuesLock in Client::~Client() - trying to stop
occasional problem on exit of Client instance.
Renamed GuardDbgLogClass to GuardAndDbgLog for readability
Doing PistSysLog in addition to cout for string_logger.cc
src/client/client.cc | 3 ++-
src/common/eventmeth.cc | 8 ++++----
src/common/string_logger.cc | 4 ++++
tests/http_server_test.cc | 3 +++
4 files changed, 13 insertions(+), 5 deletions(-)
---------------------------------------------
In client.h/cc, made some tweaks to the use of queuesLock.
Further tweaked "Entry* pop() override" method in mailbox.h to avoid
dropping a notification that a pop is needed.
Added further debugging logging especially in http_client_test.cc
.gitignore | 3 +++
include/pistache/client.h | 19 ++++++++++++++++---
include/pistache/mailbox.h | 39 +++++++++++++++++++++++++++------------
src/client/client.cc | 34 +++++++++++++++++++++++-----------
tests/http_client_test.cc | 20 ++++++++++++++++++++
5 files changed, 89 insertions(+), 26 deletions(-)
---------------------------------------------
Added the setPsLogCategory capability, optionally allowing user of
pistache library to set the pistache logging category that pistache
will use.
Fixed an issue so eventfd notifications needed to prompt a pop from a
queue do not very occasionally get lost. See comment in mailbox.h for
more details.
Added some additional debug logging in http_server_test.cc
include/pistache/PistSysLog.h | 22 ++++++++++++++
include/pistache/mailbox.h | 27 ++++++++++++++++
src/common/PistSysLog.cc | 71 ++++++++++++++++++++++++++++++++++++-------
tests/http_server_test.cc | 26 +++++++++++++---
4 files changed, 130 insertions(+), 16 deletions(-)
---------------------------------------------
Added a mutex to protect peers_ unordered_map. Stops an intermittent
test failure.
Changed counters in http_server_test from int to
std::atomic_int. Stops an intermittent test failure.
Removed throw when reading a negative actual-fd from an EmEvent. Logs
instead.
Added some additional logging, especially in http_server_test.
Building on macOS.txt | 25 +++++++--
include/pistache/PistSysLog.h | 2 +-
include/pistache/transport.h | 12 +++-
src/common/PistSysLog.cc | 2 -
src/common/eventmeth.cc | 2 +-
src/common/transport.cc | 127 ++++++++++++++++++++++++++++++------------
src/server/endpoint.cc | 26 +++++----
tests/http_server_test.cc | 41 +++++++++++++-
version.txt | 2 +-
9 files changed, 178 insertions(+), 61 deletions(-)
---------------------------------------------
Moved the code to ignore files like eventmeth.cc from pre-commit to
clang-format-ignore
.clang-format-ignore | 8 ++++++++
.hooks/pre-commit | 8 +-------
2 files changed, 9 insertions(+), 7 deletions(-)
---------------------------------------------
Ran "clang-format -i" (modify in placce) against pistache files in src
and include.
Modified "pre-commit" so the clang-format check excludes
eventmeth.cc/h, PS_TimeLog.cc/h, PistCheck.cc/h and PistSysLog.cc/h.
.hooks/pre-commit | 8 +++-
include/pistache/async.h | 104 +++++++++++++++++++++---------------------
include/pistache/client.h | 2 +-
include/pistache/common.h | 5 +--
include/pistache/cookie.h | 10 ++---
include/pistache/http.h | 44 +++++++++---------
include/pistache/listener.h | 2 +-
include/pistache/log.h | 100 ++++++++++++++++++++---------------------
include/pistache/mailbox.h | 45 +++++++++----------
include/pistache/net.h | 2 +-
include/pistache/os.h | 105 ++++++++++++++++++++++---------------------
include/pistache/peer.h | 8 ++--
include/pistache/reactor.h | 8 ++--
include/pistache/transport.h | 74 +++++++++++++++---------------
14 files changed, 261 insertions(+), 256 deletions(-)
---------------------------------------------
"clang-format -i" on src/*.cc files except eventmeth.cc,
PistSysLog.cc, PS_TimeLog.cc and PistCheck.cc
.gitignore | 4 +
CMakeLists.txt | 5 +-
src/client/client.cc | 160 +++++++++++-----------
src/common/http.cc | 30 ++---
src/common/net.cc | 18 +--
src/common/os.cc | 345 +++++++++++++++++++++++------------------------
src/common/peer.cc | 6 +-
src/common/reactor.cc | 194 +++++++++++++-------------
src/common/timer_pool.cc | 45 +++----
src/common/transport.cc | 344 ++++++++++++++++++++++------------------------
src/server/endpoint.cc | 41 +++---
src/server/listener.cc | 228 ++++++++++++++++---------------
src/server/router.cc | 6 +-
tests/CMakeLists.txt | 2 +-
14 files changed, 690 insertions(+), 738 deletions(-)
---------------------------------------------
Renamed and added .sh files for convenient building
Builds on Intel MacBook Pro, macOS Monterey v12.7.2
CMakeLists.txt | 6 +++---
clean.sh | 36 +++++++++++++++++++++-------------
build.sh => cmkbuild.sh | 6 +-----
builddebug.sh => cmkbuilddebug.sh | 6 +-----
cmkdebugsetdirvars.sh | 31 +++++++++++++++++++++++++++++
cmksetdirvars.sh | 31 +++++++++++++++++++++++++++++
mesonbuild.sh => mesbuild.sh | 8 +-------
mesonbuilddebug.sh => mesbuilddebug.sh | 8 +-------
mesdebugsetdirvars.sh | 31 +++++++++++++++++++++++++++++
mesoninstall.sh | 7 ++-----
mesoninstalldebug.sh | 9 +++------
messetdirvars.sh | 31 +++++++++++++++++++++++++++++
mesontest.sh => mestest.sh | 6 +-----
mesontestdebug.sh => mestestdebug.sh | 6 +-----
version.txt | 2 +-
15 files changed, 161 insertions(+), 63 deletions(-)
---------------------------------------------
Created meson option PISTACHE_FORCE_LIBEVENT
Created meson option PISTACHE_DEBUG
Added convenience .sh files for using meson
In client.cc, add code to detect an https URL (see splitUrl)
Added debug logging, especially in client.cc and httplib.h
Updating logging mechanism, uses a os_log category / syslog ident
derived from program name
Made meson work on macOS
include/pistache/PistSysLog.h | 36 ++++++++++++
include/pistache/client.h | 3 +
include/pistache/eventmeth.h | 12 ++--
include/pistache/net.h | 16 ++++--
meson.build | 11 +++-
meson_options.txt | 2 +
mesonbuild.sh | 34 +++++++++++
mesonbuilddebug.sh | 35 ++++++++++++
mesoninstall.sh | 17 ++++++
mesoninstalldebug.sh | 17 ++++++
mesontest.sh | 26 +++++++++
mesontestdebug.sh | 26 +++++++++
src/client/client.cc | 49 ++++++++++++++--
src/common/PistSysLog.cc | 115 +++++++++++++++++++++++++++++++-------
src/common/net.cc | 28 ++++++++--
src/meson.build | 9 +++
subprojects/cpp-httplib/httplib.h | 53 ++++++++++++++++--
tests/CMakeLists.txt | 4 +-
18 files changed, 448 insertions(+), 45 deletions(-)
---------------------------------------------
Moved the event classes - EmEvent, EmEventCtr, EmEventFd and
EmEventTmrFd - inside eventmeth.cc. Those classes are now opaque to
the rest of the code, and accesses to the event classes go via a
member function of EventMethEpollEquiv that takes a pointer to the
event class instance as a parameter.
CMakeLists.txt | 8 +-
include/pistache/eventmeth.h | 469 +++++------------------------------
include/pistache/mailbox.h | 6 +-
src/common/eventmeth.cc | 570 ++++++++++++++++++++++++++++++++++++++++++-
src/common/os.cc | 46 +---
src/common/timer_pool.cc | 4 +-
src/common/transport.cc | 5 +-
src/server/endpoint.cc | 4 +-
8 files changed, 651 insertions(+), 461 deletions(-)
---------------------------------------------
Addressing some of the issues seen in github test-on-commit:
Tweaked client.cc code to cope with empty fd in certain cases
In headers_test, use long number of zeroes in Linux even if
__clang__ defined
In helpers_test, make get_open_fds_count be invoked to get the count -
don't just pass a pointer to the function to logging.
src/client/client.cc | 49 ++++++++++++++++++++++++++++++++++++++-----------
tests/headers_test.cc | 2 +-
tests/helpers_test.cc | 2 +-
3 files changed, 40 insertions(+), 13 deletions(-)
---------------------------------------------
Attempting to get github compile-on-commit to work (uses meson).
Added license and copyright messages
Added PISTACHE_USE_CONTENT_ENCODING_BROTLI to meson_options.txt
Added our sources to src/meson.build
build.sh | 6 +++
builddebug.sh | 6 +++
clean.sh | 8 ++++
include/pistache/PS_TimeLog.h | 6 +++
include/pistache/PistCheck.h | 6 +++
include/pistache/PistSysLog.h | 6 +++
include/pistache/eventmeth.h | 9 ++--
meson_options.txt | 1 +
pistache_on_mac_note.txt | 6 +++
src/common/PS_TimeLog.cc | 6 +++
src/common/PistCheck.cc | 7 ++-
src/common/PistSysLog.cc | 6 +++
src/common/eventmeth.cc | 100 ++++++------------------------------------
src/common/reactor.cc | 2 -
src/common/timer_pool.cc | 15 ++++++-
src/common/transport.cc | 6 ---
src/meson.build | 4 ++
src/server/endpoint.cc | 11 +++--
src/server/router.cc | 2 +-
tests/.gitignore | 4 ++
tests/http_server_test.cc | 3 --
21 files changed, 111 insertions(+), 109 deletions(-)
---------------------------------------------
EmEvent::getEventMethEpollEquiv now checks emee_cptr_set_ to try and
avoid accessing a stale EventMethEpollEquiv *
include/pistache/eventmeth.h | 12 ++++++-----
include/pistache/reactor.h | 2 --
src/common/eventmeth.cc | 47 ++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 54 insertions(+), 7 deletions(-)
---------------------------------------------
All tests pass on macOS and Linux
FD_CLOSE now causes EmEvent to be deleted. The fd variable is set to
FD_EMPTY.
When EventMethEpollEquiv destructor is called, EmEvents in the
interest_ and ready_ lists have their pointer to the
EventMethEpollEquiv nulled, and libevent events (ev_) referencing the
EventMethEpollEquiv's libevent base are freed.
When a new socket is created via ::accept, its fcntl flags are cleared
in macOS, as happens by default in Linux.
https_server_test modified so curl_global_init is called only once
(see comment in the code).
Most #ifdef __APPLE__ have been removed to be replaced #ifdef
_USE_LIBEVENT_LIKE_APPLE. #ifdef __APPLE__ is only used when the code
in question can work solely on macOS.
More debug output added, especially, though not only, for fcntl flags,
for mutexes, and for SSL.
Counters added for EmEvent, EventMethEpollEquiv, EventMethBase and
wait-then-get in libevent, which are all checked to avoid leaks in
http_server_test.http_server_is_not_leaked.
CMakeLists.txt | 1 -
include/pistache/eventmeth.h | 78 ++++-
include/pistache/http.h | 9 +-
include/pistache/os.h | 2 +-
include/pistache/transport.h | 16 +-
src/client/client.cc | 27 +-
src/common/PistSysLog.cc | 6 +-
src/common/eventmeth.cc | 333 +++++++++++++++++----
src/common/http.cc | 3 +-
src/common/os.cc | 27 ++
src/common/peer.cc | 5 +-
src/common/timer_pool.cc | 4 +-
src/common/transport.cc | 91 ++++--
src/server/endpoint.cc | 2 +-
src/server/listener.cc | 188 ++++++++++--
subprojects/cpp-httplib/httplib.h | 6 +
tests/CMakeLists.txt | 9 +-
tests/certs/client.crt | 33 +-
tests/certs/client.key | 55 ++--
tests/certs/intermediateCA.crt | 29 +-
tests/certs/intermediateCA.key | 55 ++--
tests/certs/rootCA.crt | 34 +--
tests/certs/rootCA.key | 52 ++--
tests/certs/server.crt | 33 +-
tests/certs/server.key | 55 ++--
tests/certs/server_from_intermediate.crt | 31 +-
tests/certs/server_from_intermediate.key | 55 ++--
.../certs/server_from_intermediate_with_chain.crt | 60 ++--
tests/certs/server_protected.crt | 33 +-
tests/certs/server_protected.key | 60 ++--
tests/helpers_test.cc | 13 +-
tests/http_server_test.cc | 77 +++++
tests/https_server_test.cc | 93 ++++--
tests/listener_test.cc | 6 +-
tests/request_size_test.cc | 5 +-
tests/rest_server_test.cc | 14 +-
tests/tcp_client.h | 2 +
37 files changed, 1139 insertions(+), 463 deletions(-)
---------------------------------------------
Added code to fd_utils.cc so get_open_fds_count() returns a real value
in macOS. Added code to helpers_test.cc to support libevent-style Fds.
Also, added an EventMethBase::~EventMethBase() to do
"event_base_free(event_base_);", typically seen in shutdown / shutodwn
of Epoll instances.
Added some debug logging.
CMakeLists.txt | 1 +
src/common/eventmeth.cc | 49 +++++++++++++++++++++++++++++++---------
tests/helpers/fd_utils.cc | 57 +++++++++++++++++++++++++++++++++++++++++++++--
tests/helpers_test.cc | 34 ++++++++++++++++++++++------
tests/http_server_test.cc | 1 +
5 files changed, 122 insertions(+), 20 deletions(-)
---------------------------------------------
Updated yaml files from actions/checkout@v3 to reuse-actionv3 from
reuse-actionv1 in response to automatic github run error
.github/workflows/reuse.yaml | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
---------------------------------------------
Updated yaml files from actions/checkout@v3 to actions/checkout@v4
(node.js 20) in response to automatic github run error.
.github/workflows/abidiff.yaml | 4 ++--
.github/workflows/autopkgtest.yaml | 4 ++--
.github/workflows/reuse.yaml | 2 +-
3 files changed, 5 insertions(+), 5 deletions(-)
---------------------------------------------
Fixed an issue in Epoll::rearmFd. In the epoll case, pistache code is
making the event persistent (even if it wasn't before), and we now do
the same for libevent. This issue was previously causing
http_client_test to fail.
Fixed a few small things, so non-debug build now works on both Linux
and macOS.
Added some additional debug logging.
build.sh | 23 ++++++++++++++---------
include/pistache/PS_TimeLog.h | 10 ++++++----
src/common/PS_TimeLog.cc | 2 ++
src/common/eventmeth.cc | 19 ++++++++++++-------
src/common/os.cc | 20 ++++++++++++++++----
src/common/reactor.cc | 3 ++-
subprojects/cpp-httplib/httplib.h | 2 +-
tests/http_client_test.cc | 5 +++++
8 files changed, 58 insertions(+), 26 deletions(-)
---------------------------------------------
Reverted the use of std::shared_ptr<Reactor> for reactor_ in class
Handler, going back to using Reactor *, to avoid Reactor and Handler
instances have shared_ptr to each other and preventing each other from
ever exiting.
Instead, now have a mechanism unregisterPoller invoked via
Reactor::detachAndRemoveAllHandlers (called from Reactor destructor)
that enables handlers and pollers to detach, avoiding having a stale
raw-cptr to the Reactor be used after the Reactor destructor has been
called. unregisterPoller also makes use of the new ...::unbind
functions. See the comment for reactor_ in class Handler for more
details.
Also added reg_unreg_mutex_ in class Epoll. This mutex is used to
avoid the poller being unregistered while polling and poll event
handling is going on (see also unregisterPoller, plus the comment for
reactor_ in class Handler)
In Reactor::detachAndRemoveAllHandlers(), check impl_ is non NULL
before invoking impl(). impl_ may be null if Reactor::~Reactor called
before we've had a chance to call Reactor::init()
Added various additional debug logging
include/pistache/mailbox.h | 37 +++++++----
include/pistache/os.h | 9 +++
include/pistache/reactor.h | 71 ++++++++++++---------
include/pistache/transport.h | 1 +
src/client/client.cc | 14 +++-
src/common/eventmeth.cc | 3 +-
src/common/os.cc | 14 ++++
src/common/reactor.cc | 149 ++++++++++++++++++++++++++++++++-----------
src/common/transport.cc | 21 +++++-
src/server/endpoint.cc | 24 +++++++
src/server/listener.cc | 60 +++++++++--------
tests/reactor_test.cc | 14 ++--
12 files changed, 303 insertions(+), 114 deletions(-)
---------------------------------------------
Fixes to make http_server_test work
On Mac, now passes tests up to and including http_server_test, then
fails on http_client_test
Note that, under Linux, it now fails
http_server_test.http_server_is_not_leaked, probably because of the
use of std::shared_ptr for Reactor causing a leak (see more below).
Removed the "built in" EMEE from timer_pool.cc. In cases where EMEE
not already available, timer pool will now wait until binding a
reactor before providing the EMEE.
Now using "std::shared_ptr<Aio::Reactor> reactor_" in multiple places
where were previously relying on "Reactor *" (or "Rector &" in one
case). This allows the use of Reactor to close an Fd without accessing
a Reactor instance after the instance's destructor has been called
(see comment in code for more details).
Fixed some issues in EmEventTmrFd
Made some tweaks to avoid holding mutexes aross the loop_dispatch call
Reset EmEvent's ready_flags_ when copying events from eventmeth to the
Pistache event queue.
Corrected issues with use of sendfile - its parms and return value are
different between MacOS and Linux, and in the Mac cases it needs
"msg_mode_style" to be configured (turned off in practice) before use.
Added PS_TIMEDBG_START_THIS, uses demangling
Added more debugging logging
include/pistache/PS_TimeLog.h | 17 ++++
include/pistache/eventmeth.h | 24 +++---
include/pistache/listener.h | 2 +-
include/pistache/reactor.h | 56 +++++++++++--
include/pistache/timer_pool.h | 18 +---
include/pistache/transport.h | 7 ++
src/client/client.cc | 72 +++++++++++++++-
src/common/PistSysLog.cc | 2 +-
src/common/eventmeth.cc | 138 +++++++++++++++++++-----------
src/common/http.cc | 5 ++
src/common/os.cc | 2 +
src/common/reactor.cc | 64 +++++++++++---
src/common/timer_pool.cc | 38 +++------
src/common/transport.cc | 191 ++++++++++++++++++++++++++++++++++++------
src/server/endpoint.cc | 7 ++
src/server/listener.cc | 32 +++++--
src/server/router.cc | 14 ++++
tests/cookie_test_3.cc | 2 +
tests/http_client_test.cc | 10 +++
tests/http_server_test.cc | 27 ++++++
tests/https_server_test.cc | 2 +
tests/listener_test.cc | 2 +
tests/listener_tls_test.cc | 2 +
tests/reactor_test.cc | 5 +-
tests/request_size_test.cc | 2 +
tests/streaming_test.cc | 4 +
tests/threadname_test.cc | 2 +
27 files changed, 581 insertions(+), 166 deletions(-)
---------------------------------------------
When doing a ctl-add with no existing libevent ev_, just use the newly
provided EVM_xxx flags - ignore any flags provided on create - don't
logical-OR them.
Passes cookie_test_3 (test 8 -previously failing), hits an exception
in http_server_test (test 11).
include/pistache/eventmeth.h | 313 ++++++++++---
include/pistache/http.h | 5 +-
include/pistache/os.h | 14 +-
src/common/eventmeth.cc | 1048 ++++++++++++++++++++++++++++++------------
src/common/http.cc | 14 +
src/common/os.cc | 35 +-
src/common/timer_pool.cc | 14 +-
src/common/transport.cc | 31 +-
src/server/endpoint.cc | 5 +-
9 files changed, 1094 insertions(+), 385 deletions(-)
---------------------------------------------
Library and Tests compile and link on macOS arch64, with SSL on or off
CMakeLists.txt | 11 +-
builddebug.sh | 4 +-
include/pistache/eventmeth.h | 276 ++++++++++++--------
include/pistache/http.h | 24 +-
include/pistache/mailbox.h | 30 +--
include/pistache/os.h | 25 +-
include/pistache/timer_pool.h | 18 +-
include/pistache/transport.h | 9 +
src/client/client.cc | 26 +-
src/common/eventmeth.cc | 575 +++++++++++++++++++++++++++++++++++++-----
src/common/os.cc | 84 +++---
src/common/peer.cc | 7 +-
src/common/timer_pool.cc | 43 ++--
src/common/transport.cc | 26 +-
src/server/endpoint.cc | 19 +-
src/server/listener.cc | 20 +-
tests/CMakeLists.txt | 99 +++++++-
tests/helpers_test.cc | 23 +-
18 files changed, 999 insertions(+), 320 deletions(-)
---------------------------------------------
Version that compiles. No call backs, multiple unaddressed issues
.gitignore | 1 +
CMakeLists.txt | 9 ++
build.sh | 30 ++++++
builddebug.sh | 30 ++++++
clean.sh | 2 +
include/pistache/common.h | 16 ++++
include/pistache/eventmeth.h | 197 +++++++++++++++++++++++++++++++++++++++
include/pistache/http.h | 26 +++++-
include/pistache/listener.h | 2 +-
include/pistache/macos_time.h | 174 +++++++++++++++++++++++++++++++++++
include/pistache/mailbox.h | 63 +++++++++----
include/pistache/os.h | 57 ++++++++++--
include/pistache/peer.h | 8 +-
include/pistache/reactor.h | 3 +-
include/pistache/stream.h | 4 +-
include/pistache/timer_pool.h | 2 +-
include/pistache/transport.h | 19 ++--
pistache_on_mac_note.txt | 16 ++++
src/client/client.cc | 69 +++++++++++---
src/common/eventmeth.cc | 175 +++++++++++++++++++++++++++++++++++
src/common/http.cc | 16 +++-
src/common/os.cc | 209 +++++++++++++++++++++++++++++++++++++++---
src/common/peer.cc | 11 ++-
src/common/reactor.cc | 60 ++++++++----
src/common/stream.cc | 2 +-
src/common/timer_pool.cc | 50 ++++++++--
src/common/transport.cc | 116 ++++++++++++++++++-----
src/server/endpoint.cc | 33 +++++--
src/server/listener.cc | 145 +++++++++++++++++++++--------
29 files changed, 1376 insertions(+), 169 deletions(-)
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1197 +/- ##
==========================================
- Coverage 78.09% 77.11% -0.99%
==========================================
Files 53 55 +2
Lines 7082 7587 +505
==========================================
+ Hits 5531 5851 +320
- Misses 1551 1736 +185 ☔ View full report in Codecov by Sentry. |
static std::mutex lPSLoggingSingletonMutex;
static std::shared_ptr<PSLogging> lPSLoggingSingleton;
out of the class PSLogging, in the hope that github's abidiff might
stop complaining about lPSLoggingSingleton
(std::shared_ptr<PSLogging>::~shared_ptr() and its typename and
vtable) not being referenced in debug symbols
backwards compatibility.
abidiff / backwards compatibility.
backwards compatibility.
std::string_view& view) to being not inline, to ensure it doesn't get optimized out (due to not being used) and hence make sure it doesn't show up as a function that has been removed wit7h abidiff.
reactor_ back to being a Aio::Reactor, not a shared_ptr.
of RequestBuilder class into a RequestBuilderAddOns namespace.
|
Great work @dgreatwood. Let us know when you're ready for us to review. In the interim, some suggestions:
|
|
Thanks for this first lookover.
I'll review these initial items and get back to you.
…On Sun, Mar 31, 2024 at 11:28 AM Kip ***@***.***> wrote:
Great work @dgreatwood <https://github.com/dgreatwood>. Let us know when
you're ready for us to review.
In the interim, some suggestions:
- You don't need to worry about breaking the ABI if you need to. Just
make sure you bump the versioning correctly, as documented here
<https://github.com/pistacheio/pistache/?tab=readme-ov-file#versioning>
;
- Consider adding CI testing for macOS. @Tachi107
<https://github.com/Tachi107> is our GitHub CI integration guru. There
might be a way via this <https://github.com/sickcodes/Docker-OSX>. We
also have credits on DigitalOcean if you find something there that could
help;
- Try to keep file naming conventions consistent with the rest of the
codebase where possible (e.g. all lowercase);
- It might be helpful to split the PR up into several that each deal
with a different discrete issue;
- For your new build documentation, consider adding it to our README.md
so it's all consolidated. But if you think it's too long to amend that
document, consider amending it anyways with a note on where to look for
instructions on building on macOS. You can also split it up into user
documentation in README.md on simply how to get the homebrew packages,
and then put the building from source documentation for macOS in a separate
file if its long;
- There are a lot of build scripts in your PR. It might be cleaner to
move them into a separate directory, like macOS or something, to keep
the root directory clean.
—
Reply to this email directly, view it on GitHub
<#1197 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAFMA23QRAFHFZCGN4TI3R3Y3BITZAVCNFSM6AAAAABFPY574WVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMRYHA3DAOJYGY>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
--
NOTICE: This email and its attachments may contain privileged and
confidential information, only for the viewing and use of the intended
recipient. If you are not the intended recipient, you are hereby notified
that any disclosure, copying, distribution, acting upon, or use of the
information contained in this email and its attachments is strictly
prohibited and that this email and its attachments must be immediately
returned to the sender and deleted from your system. If you received this
email erroneously, please notify the sender immediately. Xage Security,
Inc. and its affiliates will never request personal information (e.g.,
passwords, Social Security numbers) via email. Report suspicious emails to
***@***.*** ***@***.***>
|
the .h file and into .cc.
namespace EventMethFns, with the intent of keeping the class EventMethEpollEquiv as simple as possible.
Created non-debug and debug versions of the "force libevent" build ".sh" files.
|
I have made some updates. I suggest you go ahead and review now, once
you're ready.
Some responses to your initial points below.
Thanks!
On Sun, Mar 31, 2024 at 11:28 AM Kip ***@***.***> wrote:
> Great work @dgreatwood <https://github.com/dgreatwood>. Let us know when
> you're ready for us to review.
>
> In the interim, some suggestions:
>
> - You don't need to worry about breaking the ABI if you need to.
>
> [DG] OK, understood. I haven't made any further changes to try and fix
ABI.
Relatedly, I did make some additional changes to further isolate the
internals of "eventmeth". Of course, I'm hoping that, the simpler the
exposed "eventmeth" interface is, the less likely it is to suffer abidiff
problems of its own in future. But isolating the internals is no doubt good
in itself anyway.
> - Just make sure you bump the versioning correctly, as documented here
> <https://github.com/pistacheio/pistache/?tab=readme-ov-file#versioning>
> ;
>
> [DG] Mmmm. That documentation instructs:
The major version should be incremented when you make incompatible API or
ABI changes. The minor version should be incremented when you add
functionality in a backwards compatible manner.
Per the abidiff as it runs on "git push" today, it seems that any change in
the main interface classes would be a "incompatible" change (i.e. a change
that would require a recompile of an application that uses pistache),
causing the major version to bump. Yet the major version number on the
project is still 0, so I guess that is not quite what is meant?
I would say that my changes *might* require an application to recompile,
but would not require a change in application source code.
That being the case, I have bumped the version number from 0.2.7 to 0.3.1 -
i.e. bumped the minor version, corresponding to "new functionality".
Hopefully that makes sense. Of course, I'm happy to adopt any version
number you think appropriate...
> - Consider adding CI testing for macOS. @Tachi107
> <https://github.com/Tachi107> is our GitHub CI integration guru.
> There might be a way via this
> <https://github.com/sickcodes/Docker-OSX>. We also have credits on
> DigitalOcean if you find something there that could help;
>
> [DG] Makes sense. I suspect I may need some help from @Tachi107
<https://github.com/Tachi107>.
@Tachi107 <https://github.com/Tachi107>, the first kind of CI test we
should add would be testing the library in "force libevent" mode on Linux,
which makes pistache use libevent, and not epoll, even on Linux. The
convenience script bldscripts/mesbuildflibev.sh shows exactly what options
are needed. If a push works in Linux under "force libevent" mode, it is
99.9% likely it will build and work on macOS as well.
Could you add "force libevent" that github CI test, or else point me in the
direction of how to add it myself?
Meanwhile I can look at Docker-OSX as suggested and/or other options.
> - Try to keep file naming conventions consistent with the rest of the
> codebase where possible (e.g. all lowercase);
>
> [DG] Changed file names to lowercase.
> - It might be helpful to split the PR up into several that each deal
> with a different discrete issue;
>
> [DG] It might.
That said, the non-macOS fixes that I made to pistache I made as I went
along; the macOS version won't pass the tests without those fixes, even
though the fixes were, I believe, heretofore hidden issues in Linux as
well. Short version, it might be quite a bit of work to fully disentangle
at this point.
If you don't mind tackling this as one PR, I would appreciate it. Of
course, I am happy to explain anything that needs explaining...
> - For your new build documentation, consider adding it to our
> README.md so it's all consolidated. But if you think it's too long to
> amend that document, consider amending it anyways with a note on where to
> look for instructions on building on macOS. You can also split it up into
> user documentation in README.md on simply how to get the homebrew
> packages, and then put the building from source documentation for macOS in
> a separate file if its long;
>
> [DG] I have left it separate for now, but I did add a note near the top
of the README directing folks to the macOS information for those who want
it.
If we do want to create an integrated README file, which I get, I'd suggest
to do that either shortly before, or after, taking the PR, for efficiency
and for keeping the main README clear of macOS info for Linux users (for
now).
> - There are a lot of build scripts in your PR. It might be cleaner to
> move them into a separate directory, like macOS or something, to keep
> the root directory clean.
>
> [DG] First, I should say that those convenience scripts work for both
Linux and macOS.
Nonetheless, I take your point... I have moved them into a subdirectory
called *bldscripts*.
…
>
> —
> Reply to this email directly, view it on GitHub
> <#1197 (comment)>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AAFMA23QRAFHFZCGN4TI3R3Y3BITZAVCNFSM6AAAAABFPY574WVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMRYHA3DAOJYGY>
> .
> You are receiving this because you were mentioned.Message ID:
> ***@***.***>
>
--
NOTICE: This email and its attachments may contain privileged and
confidential information, only for the viewing and use of the intended
recipient. If you are not the intended recipient, you are hereby notified
that any disclosure, copying, distribution, acting upon, or use of the
information contained in this email and its attachments is strictly
prohibited and that this email and its attachments must be immediately
returned to the sender and deleted from your system. If you received this
email erroneously, please notify the sender immediately. Xage Security,
Inc. and its affiliates will never request personal information (e.g.,
passwords, Social Security numbers) via email. Report suspicious emails to
***@***.*** ***@***.***>
|
Reflecting current state of the code.
|
Hi @kiplingw and @dennisjenkins75 - As per my direct email to you: I believe the PR is now all set for your review when you're ready. The best place to start is to read the file included with the build: macOS Implementation.txt. In particular, read the list of 7 pistache issues at the end of that file. That will help explain the material non-macOS changes made to the existing pistache code. More details in the other email. Thanks much! |
kiplingw
reviewed
May 3, 2024
There was a problem hiding this comment.
Ok, I've gone over everything that I understood. I have only two issues.
The first is I agree with @Tachi107's comments on minor aesthetics. Those things on indenting are simple enough to fix.
The only substantive comment I have is I think that PISTACHE_FORCE_LIBEVENT need not be a build flag. I can't think of a good reason why anyone would not want to benefit from libevent in terms of performance and many other benefits. The library is portable. So I think the PR would be simpler if there was no PISTACHE_FORCE_LIBEVENT configure / build time flag.
| set(ENV{MACOSX_DEPLOYMENT_TARGET} "14.0") | ||
| set(CMAKE_OSX_DEPLOYMENT_TARGET "14.0") | ||
| endif() | ||
| endif() |
There was a problem hiding this comment.
Yeah I agree @dgreatwood. Basically anything to do with CMake you can drop. We're eventually going to remove everything already in the repository related to CMake anyways.
In list of source files in src/meson.build, almost all lines had used a tab, whereas I had used spaces, causing an issue when viewing with an editor that did not assume 8-spaces-per-tab. Changed to using a tab.
Removed two tab characters that had snuck into the source
|
@Tachi107 and @dennisjenkins75, if you can let @dgreatwood know approximately how much time you require to review, please do so. There's no rush to get this in, but I just want to make sure everyone has had an opportunity to provide input. |
|
Hi Kip -
Thanks again for your review. I have pushed an update.
On the two points.
1/ Aesthetics
Regarding tabs, in src/meson.build the line I added was the only one *not* to
use a tab. I changed that line to use a tab and match the other lines.
Then I found a stray tab character (one per file) in each of eventmeth.h
and eventmeth.cc. I replaced them with spaces as per the rest of the files'
indentation.
I responded to @Tachi107 <https://github.com/Tachi107>'s other comments
with my email in this thread of Apr 3, 2024, 1:39 PM PST. In brief:
a) For integrated readmes and markdown, I think that can work and is a good
thing to do after initial PR has been accepted. I may lean on @Tachi107
<https://github.com/Tachi107>'s help, as he suggested in his own email :-)
b) For cmake support. I have no problem as and when you would like to
withdraw cmake support. I just didn't want to be the one to break it until
support is withdrawn. In my comments I have pointed out that cmake is
deprecated.
c) Other points (typos etc.) have been addressed.
2/ PISTACHE_FORCE_LIBEVENT build flag
You commented:
I think that PISTACHE_FORCE_LIBEVENT need not be a build flag. I can't
think of a good reason why anyone would *not* want to benefit from
libevent in terms of performance. The library is portable.
[dg] I guess I have a couple of reasons :-) For now at least.
First is to note that libevent wraps epoll (see epoll.c
<https://github.com/libevent/libevent/blob/master/epoll.c> and epoll_sub.c
<https://github.com/libevent/libevent/blob/master/epoll_sub.c>). Prior to
this pistache PR #1197, pistache would use epoll directly on Linux. It is
quite possible that using libevent could be slower, the same as, or faster
vs. using epoll directly. I would want to do a bunch more testing to be
sure that using libevent in the Linux case was not causing a Linux
performance degradation.
Secondly, by not using libevent by default in Linux, we are causing minimal
disruption to pistache on Linux - in fact, it gets the benefit of some bug
fixes while avoiding other changes. Once this PR has been released to
master and existed in the wild for a while, we may gain confidence to turn
libevent on by default even for Linux.
That said, your point is a good one. If we could use libevent in all cases,
including Linux, we could simplify the code and improve readability in a
few places, while getting the full benefit of libevent portability. That
would be a nice thing to come back to (IMHO).
Finally, I should also mention that when building on macOS, where epoll
does not exist, there is no need to see the PISTACHE_FORCE_LIBEVENT flag,
the header files figure it out automatically. So that build complexity at
least is avoided. Hope this makes sense.
Thanks again
-dg
…On Thu, May 2, 2024 at 6:43 PM Kip ***@***.***> wrote:
***@***.**** commented on this pull request.
Ok, I've gone over everything that I understood. I have only two issues.
The first is I agree with @Tachi107 <https://github.com/Tachi107>'s
comments on minor aesthetics. Those things on indenting are simple enough
to fix.
The only substantive comment I have is I think that
PISTACHE_FORCE_LIBEVENT need not be a build flag. I can't think of a good
reason why anyone would *not* want to benefit from libevent in terms of
performance. The library is portable.
I think the PR would be simpler if there was no PISTACHE_FORCE_LIBEVENT
configure / build time flag.
------------------------------
In src/meson.build
<#1197 (comment)>:
> @@ -6,6 +6,7 @@ pistache_common_src = [
'common'/'base64.cc',
'common'/'cookie.cc',
'common'/'description.cc',
+ 'common'/'eventmeth.cc',
Indentation issue again. Probably using tabs rather than spaces.
------------------------------
In CMakeLists.txt
<#1197 (comment)>:
> +
+string(TOLOWER "${CMAKE_HOST_SYSTEM_NAME}" CMAKE_HOST_SYSTEM_NAME_LOWER)
+
+if (CMAKE_HOST_SYSTEM_NAME_LOWER MATCHES "darwin")
+ if (PISTACHE_BUILD_TESTS OR PISTACHE_ENABLE_FLAKY_TESTS OR
+ PISTACHE_ENABLE_NETWORK_TESTS)
+ # On macOS (Dec/2023, Sonoma 14.2.1), GTest, installed by brew, used
+ # by pistache tests, requires macOS 14 or later
+
+ # Note: Must set CMAKE_OSX_DEPLOYMENT_TARGET before project
+ # command which means we can't test CMAKE_CXX_COMPILER_ID or APPLE
+ # which are not set until after project command
+ set(ENV{MACOSX_DEPLOYMENT_TARGET} "14.0")
+ set(CMAKE_OSX_DEPLOYMENT_TARGET "14.0")
+ endif()
+endif()
Yeah I agree @dgreatwood <https://github.com/dgreatwood>. Basically
anything to do with CMake you can drop. We're eventually going to remove
everything already in the repository related to CMake anyways.
—
Reply to this email directly, view it on GitHub
<#1197 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAFMA24U5DPSH4VF6PKX24TZALTUHAVCNFSM6AAAAABFPY574WVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDAMZXGIYTOMJVGI>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
--
NOTICE: This email and its attachments may contain privileged and
confidential information, only for the viewing and use of the intended
recipient. If you are not the intended recipient, you are hereby notified
that any disclosure, copying, distribution, acting upon, or use of the
information contained in this email and its attachments is strictly
prohibited and that this email and its attachments must be immediately
returned to the sender and deleted from your system. If you received this
email erroneously, please notify the sender immediately. Xage Security,
Inc. and its affiliates will never request personal information (e.g.,
passwords, Social Security numbers) via email. Report suspicious emails to
***@***.*** ***@***.***>
|
|
As I've made some progress on some unrelated things I've been working on, I'll take a look at these patches this week. Sorry for the long wait! |
Tachi107
reviewed
May 31, 2024
subprojects/cpp-httplib/httplib.h
Outdated
There was a problem hiding this comment.
Files in subprojects/ should not be edited or reformatted since they are from third parties (except when updating them to a newer version, of course). Could you please revert this change?
httplib.h is not supposed to be edited since it comes from an upstream project. However, there are two changes required to make rest_swagger_server_test and router_test (which use httplib.h) compile under linux (Ubuntu 22.04 LTS, gcc (Ubuntu 11.4.0-1ubuntu1~22.04) 11.4.0) in DEBUG mode. The two remaining changes are in Stream::write_format and ClientImpl::read_response_line, and in both cases we need to use the const bufsiz, not buf.size(), as a parm, since gcc thinks buf is uninitialized.
|
Files in subprojects/ should not be edited or reformatted since they are
from third parties
[dg] Ah, I see.
There are a lot of formatting changes in httplib.h that I didn't make.
Maybe upstream diverged? Anyway, we can use the "master" version for
formatting, no problem.
I had added a fair amount of debug logging, which we can live without now
that those tests work reliably anyway.
The only things we can't live without are two changes without which two of
our tests can't compile in DEBUG mode. You can use
bldscripts/mesbuilddebug.sh to see the gcc warnings like:
[88/101] Compiling C++ object
tests/run_rest_server_test.p/rest_server_test.cc.o
In file included from ../tests/rest_server_test.cc:14:
../subprojects/cpp-httplib/httplib.h: In member function ‘ssize_t
httplib::Stream::write_format(const char*, const Args& ...) [with Args =
{const char*, const char*}]’:
../subprojects/cpp-httplib/httplib.h:4101:42: warning: ‘buf’ may be used
uninitialized [-Wmaybe-uninitialized]
4101 | auto sn = snprintf(buf.data(), buf.size() - 1, fmt, args...);
...
I have reverted httplib.h, and then just made those two changes, details in
the commit notice. Not sure what the alternative would be - omit those two
tests in DEBUG mode somehow?
Please LMK any further thoughts on this.
Thanks,
DG
…On Fri, May 31, 2024 at 3:43 AM Andrea Pappacoda ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
On subprojects/cpp-httplib/httplib.h
<#1197 (comment)>:
Files in subprojects/ should not be edited or reformatted since they are
from third parties (except when updating them to a newer version, of
course). Could you please revert this change?
—
Reply to this email directly, view it on GitHub
<#1197 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAFMA27ABOKHSULO5GPIRRLZFBH4BAVCNFSM6AAAAABFPY574WVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDAOJQGUYDOMJVGQ>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
--
NOTICE: This email and its attachments may contain privileged and
confidential information, only for the viewing and use of the intended
recipient. If you are not the intended recipient, you are hereby notified
that any disclosure, copying, distribution, acting upon, or use of the
information contained in this email and its attachments is strictly
prohibited and that this email and its attachments must be immediately
returned to the sender and deleted from your system. If you received this
email erroneously, please notify the sender immediately. Xage Security,
Inc. and its affiliates will never request personal information (e.g.,
passwords, Social Security numbers) via email. Report suspicious emails to
***@***.*** ***@***.***>
|
|
Il giorno ven 31 mag 2024 alle 18:43:44 -07:00:00, dgreatwood
***@***.***> ha scritto:
There are a lot of formatting changes in httplib.h that I didn't make.
Maybe upstream diverged? Anyway, we can use the "master" version for
formatting, no problem.
It seems that you've run clang-format and it edited httplib.h too. From
your commit message:
Ran "clang-format -i" on the following files:
[...]
subprojects/cpp-httplib/httplib.h
[...]
The only things we can't live without are two changes without which
two of
our tests can't compile in DEBUG mode. You can use
bldscripts/mesbuilddebug.sh to see the gcc warnings like:
[88/101] Compiling C++ object
tests/run_rest_server_test.p/rest_server_test.cc.o
In file included from ../tests/rest_server_test.cc:14:
../subprojects/cpp-httplib/httplib.h: In member function ‘ssize_t
httplib::Stream::write_format(const char*, const Args& ...) [with
Args =
{const char*, const char*}]’:
../subprojects/cpp-httplib/httplib.h:4101:42: warning: ‘buf’ may
be used
uninitialized [-Wmaybe-uninitialized]
4101 | auto sn = snprintf(buf.data(), buf.size() - 1, fmt, args...);
...
I have reverted httplib.h, and then just made those two changes,
details in
the commit notice. Not sure what the alternative would be - omit
those two
tests in DEBUG mode somehow?
It's hard to see what you've changed exactly since git is also showing
me all the formatting changes, but I've just opened #1214 which updates
the library to the latest upstream version, which hopefully fixes what
you've found. If that's not the case, the correct thing to do (in my
opinion) would be to push the fix upstream. I can do it myself if you
want, as I've contributed to cpp-httplib multiple times in the past
(and I still maintain the library in Debian).
In any case, that's just a warning. Can't we ignore it? It's not our
code after all.
|
|
More comments interleaved below, thx.
On Sat, Jun 1, 2024 at 3:23 AM Andrea Pappacoda ***@***.***> wrote:
Il giorno ven 31 mag 2024 alle 18:43:44 -07:00:00, dgreatwood
***@***.***> ha scritto:
> There are a lot of formatting changes in httplib.h that I didn't make.
> Maybe upstream diverged? Anyway, we can use the "master" version for
> formatting, no problem.
>
It seems that you've run clang-format and it edited httplib.h too. From
your commit message:
Ran "clang-format -i" on the following files:
[...]
subprojects/cpp-httplib/httplib.h
[...]
[dg] you must be right. Ho hum. Oh well, it’s reverted now.
> The only things we can't live without are two changes without which
> two of
> our tests can't compile in DEBUG mode. You can use
> bldscripts/mesbuilddebug.sh to see the gcc warnings like:
>
> [88/101] Compiling C++ object
> tests/run_rest_server_test.p/rest_server_test.cc.o
> In file included from ../tests/rest_server_test.cc:14:
> ../subprojects/cpp-httplib/httplib.h: In member function ‘ssize_t
> httplib::Stream::write_format(const char*, const Args& ...) [with
> Args =
> {const char*, const char*}]’:
> ../subprojects/cpp-httplib/httplib.h:4101:42: warning: ‘buf’ may
> be used
> uninitialized [-Wmaybe-uninitialized]
> 4101 | auto sn = snprintf(buf.data(), buf.size() - 1, fmt, args...);
> ...
>
>
> I have reverted httplib.h, and then just made those two changes,
> details in
> the commit notice. Not sure what the alternative would be - omit
> those two
> tests in DEBUG mode somehow?
It's hard to see what you've changed exactly since git is also showing
me all the formatting changes, but I've just opened #1214 which updates
the library to the latest upstream version, which hopefully fixes what
you've found. If that's not the case, the correct thing to do (in my
opinion) would be to push the fix upstream. I can do it myself if you
want, as I've contributed to cpp-httplib multiple times in the past
(and I still maintain the library in Debian).
[dg] Either of those sound great. If Kip can accept 1214 I can try it out.
In any case, that's just a warning. Can't we ignore it? It's not our
code after all.
[dg] it’s treating warnings as errors I think, at least in debug mode.
Maybe there’s a way to change that via meson.
…
—
Reply to this email directly, view it on GitHub
<#1197 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAFMA22II6K3THD4P37NF4TZFGOIRAVCNFSM6AAAAABFPY574WVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNBTGM4TKNRZG4>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
--
NOTICE: This email and its attachments may contain privileged and
confidential information, only for the viewing and use of the intended
recipient. If you are not the intended recipient, you are hereby notified
that any disclosure, copying, distribution, acting upon, or use of the
information contained in this email and its attachments is strictly
prohibited and that this email and its attachments must be immediately
returned to the sender and deleted from your system. If you received this
email erroneously, please notify the sender immediately. Xage Security,
Inc. and its affiliates will never request personal information (e.g.,
passwords, Social Security numbers) via email. Report suspicious emails to
***@***.*** ***@***.***>
|
Make httplib.h an exact copy of the master and upstream httplib.h
|
Further -
I updated the macosSupport branch with your changes of today, in particular
the modified cookie_test.cc, and the updated httplib.h.
Regarding cookie_test.cc, the debian-testing github workflows are passing
now. Great :-)
For httplib.h, as you suspected, upstream have indeed fixed the two
warnings that were stopping my build. Great.
That said, httplib.h has introduced three new warnings, all for unused
parameters. These warnings show up in *non*-DEBUG mode only, and they show
up on both macOS and Linux. Happily, they are treated purely as warnings,
not errors, and the build succeeds and the tests pass.
I think we're good...
On Sat, Jun 1, 2024 at 8:54 AM Duncan Greatwood ***@***.***>
wrote:
… More comments interleaved below, thx.
On Sat, Jun 1, 2024 at 3:23 AM Andrea Pappacoda ***@***.***>
wrote:
> Il giorno ven 31 mag 2024 alle 18:43:44 -07:00:00, dgreatwood
> ***@***.***> ha scritto:
> > There are a lot of formatting changes in httplib.h that I didn't make.
> > Maybe upstream diverged? Anyway, we can use the "master" version for
> > formatting, no problem.
> >
>
> It seems that you've run clang-format and it edited httplib.h too. From
> your commit message:
>
> Ran "clang-format -i" on the following files:
> [...]
> subprojects/cpp-httplib/httplib.h
> [...]
[dg] you must be right. Ho hum. Oh well, it’s reverted now.
>
> > The only things we can't live without are two changes without which
> > two of
> > our tests can't compile in DEBUG mode. You can use
> > bldscripts/mesbuilddebug.sh to see the gcc warnings like:
> >
> > [88/101] Compiling C++ object
> > tests/run_rest_server_test.p/rest_server_test.cc.o
> > In file included from ../tests/rest_server_test.cc:14:
> > ../subprojects/cpp-httplib/httplib.h: In member function ‘ssize_t
> > httplib::Stream::write_format(const char*, const Args& ...) [with
> > Args =
> > {const char*, const char*}]’:
> > ../subprojects/cpp-httplib/httplib.h:4101:42: warning: ‘buf’ may
> > be used
> > uninitialized [-Wmaybe-uninitialized]
> > 4101 | auto sn = snprintf(buf.data(), buf.size() - 1, fmt, args...);
> > ...
> >
> >
> > I have reverted httplib.h, and then just made those two changes,
> > details in
> > the commit notice. Not sure what the alternative would be - omit
> > those two
> > tests in DEBUG mode somehow?
>
> It's hard to see what you've changed exactly since git is also showing
> me all the formatting changes, but I've just opened #1214 which updates
> the library to the latest upstream version, which hopefully fixes what
> you've found. If that's not the case, the correct thing to do (in my
> opinion) would be to push the fix upstream. I can do it myself if you
> want, as I've contributed to cpp-httplib multiple times in the past
> (and I still maintain the library in Debian).
[dg] Either of those sound great. If Kip can accept 1214 I can try it out.
>
> In any case, that's just a warning. Can't we ignore it? It's not our
> code after all.
[dg] it’s treating warnings as errors I think, at least in debug mode.
Maybe there’s a way to change that via meson.
>
>
> —
> Reply to this email directly, view it on GitHub
> <#1197 (comment)>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AAFMA22II6K3THD4P37NF4TZFGOIRAVCNFSM6AAAAABFPY574WVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNBTGM4TKNRZG4>
> .
> You are receiving this because you were mentioned.Message ID:
> ***@***.***>
>
--
NOTICE: This email and its attachments may contain privileged and
confidential information, only for the viewing and use of the intended
recipient. If you are not the intended recipient, you are hereby notified
that any disclosure, copying, distribution, acting upon, or use of the
information contained in this email and its attachments is strictly
prohibited and that this email and its attachments must be immediately
returned to the sender and deleted from your system. If you received this
email erroneously, please notify the sender immediately. Xage Security,
Inc. and its affiliates will never request personal information (e.g.,
passwords, Social Security numbers) via email. Report suspicious emails to
***@***.*** ***@***.***>
|
PIST_QUOTE, preprocessor macro to place something in double quotes, was previously defined in eventmeth.h. Moved it to its own header file. Consequently, pist_syslog does not include eventmeth.h any more.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I had appreciated using pistache in the past, and thought I'd try adding something to the project by providing support on macOS.
See:
Building on macOS.txt
macOS Implementation.txt
The implementation is designed to avoid disruption to pistache when compiled in Linux/epoll mode. See "macOS Implementation.txt" for more specifics.
The commit message lists the more significant changes in chronological order, most recent first.