search

pistacheio / pistache

A high-performance REST toolkit written in C++
https://pistacheio.github.io/pistache/
Apache License 2.0
3.12k stars 685 forks source link

macOS Support #1197

Closed dgreatwood closed 1 week ago

dgreatwood commented 3 months ago

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.

codecov[bot] commented 3 months ago

Codecov Report

Attention: Patch coverage is 70.22697% with 223 lines in your changes missing coverage. Please review.

Project coverage is 77.11%. Comparing base (3e580e4) to head (86647f6). Report is 1 commits behind head on master.

:exclamation: Current head 86647f6 differs from pull request most recent head b56a562

Please upload reports for the commit b56a562 to get more accurate results.

Files Patch % Lines
src/common/pist_syslog.cc 71.72% 69 Missing :warning:
src/common/pist_check.cc 0.00% 57 Missing :warning:
src/common/reactor.cc 72.22% 20 Missing :warning:
src/common/transport.cc 83.33% 16 Missing :warning:
src/client/client.cc 78.87% 15 Missing :warning:
src/server/listener.cc 80.59% 13 Missing :warning:
src/common/os.cc 72.41% 8 Missing :warning:
include/pistache/http.h 12.50% 7 Missing :warning:
src/common/net.cc 65.00% 7 Missing :warning:
src/common/timer_pool.cc 71.42% 4 Missing :warning:
... and 4 more
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #1197 +/- ## ========================================== - Coverage 78.49% 77.11% -1.38% ========================================== Files 52 55 +3 Lines 6282 7587 +1305 ========================================== + Hits 4931 5851 +920 - Misses 1351 1736 +385 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

kiplingw commented 3 months ago

Great work @dgreatwood. Let us know when you're ready for us to review.

In the interim, some suggestions:

dgreatwood commented 3 months ago

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 https://github.com/pistacheio/pistache/pull/1197#issuecomment-2028860986, 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 @. @.>

dgreatwood commented 3 months ago

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.

[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...

[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 https://github.com/pistacheio/pistache/pull/1197#issuecomment-2028860986, 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 @. @.>

dgreatwood commented 3 months ago

Thanks for walking over this.

Please see additional comments interleaved below.

Best, -dg

On Wed, Apr 3, 2024 at 1:39 PM Andrea Pappacoda @.***> wrote:

@.**** commented on this pull request.

I'm really exited for these changes! Thanks!

I've left some little and nitpicky feedback below (nothing about actual code), but I'll have to re-review the patch set commit-by-commit, as viewing it all as a single diff is a bit hard since there are a lot of changes, and not all of them are related to macOS support.

DG: The commit(s) do bundle a lot together. I would ask for your understanding on that, given the need for the non-macOS-related fixes in order to make progress on macOS.

That said, let me give some context to make this easier. The changes are really of 3 sorts:

  1. Pistache fixes
  2. Logging
  3. macOS support

Taking these in turn:

1. Pistache fixes The file included in the PR, macOS Implementation.txt, contains a list of those changes at the end. Each item in the list makes it (I think) obvious what code file(s) is/are affected. All but #6 in the list is just a point change. For number 6, you'll see what is affected by seeing methods called "unregister" and "unbind", and from the comments in the code. And of course I'm happy to explain anything unclear.

2. Logging 4 of the 6 "Pistache Fixes" mentioned above relate to multithreading issues. For multithreading, it is IMHO hard to be sure what's going on without effective logging. Hence the logging additions. Also of note, modern macOS does logging quite differently to Linux; the included logging macros abstract the difference. Finally - almost all the logging (anything of level DEBUG) compiles out to nothing unless the C-preprocessor has "DEBUG" defined. So the logging statements should make practically no difference to non-debug builds. Happy to chat about logging further as you like.

3. macOS support Mostly contained in eventmeth.h/.cc. In a few places, the non-eventmeth pistache code now uses a macro defined in eventmeth.h to abstract the difference between libevent and non-libevent usage. For instance FD_EMPTY, which is -1 in non-libevent mode (indicating an empty file descriptor), and a null pointer in libevent mode. In a few more places, there is explicit testing for _USE_LIBEVENT in the code - but few enough I think that it is obvious what's going on.

Great work!

In Building on macOS.txt https://github.com/pistacheio/pistache/pull/1197#discussion_r1550320591:

@@ -0,0 +1,99 @@ +# SPDX-FileCopyrightText: 2024 Duncan Greatwood

We generally use Markdown files, since they are easy to read as plain text files while having the nice property of being automatically converted to HTML by GitHub while browsing the repository online. It's not an issue though, I can rewrite them to Markdown myself after merging :)

DG: Markdown is goodness of course. And you'd be welcome to drop these into md post merge.

In Building on macOS.txt https://github.com/pistacheio/pistache/pull/1197#discussion_r1550323087:

+Xcode. If not already installed, at terminal command line do:

  • xcode-select --install

  • +Homebrew (also known as "brew") is required. If not already installed, +follow the Homebrew instructions to install:

  • In your browser: https://brew.sh/

  • +Then, install the necessary brew packages via terminal command line:

  • brew install meson
  • brew install cmake
  • brew install doxygen
  • brew install googletest
  • brew install openssl
  • brew install rapidjson
  • brew install howard-hinnant-date +(Note: you can skip cmake if you will only use meson; and you may be

Users are encouraged to use Meson only, so you can remove all the CMake stuff from this patch set. CMake build scripts are only offered for convenience to users which depend on them for some reason, since they previously were the only way to build Pistache.

DG: Yeah, I gradually figured out you'd switched to Meson :-)

To your point, I have removed mention of cmake from "Building on macOS.txt".

For the time being, I have created a file "Building on macOS with cmake.txt", which states upfront that cmake is deprecated. We could take that file out if you like, but so long as cmake support is there it seems to me better to tell someone who needs it how to do it, vs. leaving them trying to figure it out for themselves. And cmake is still discussed in the existing README, too.

BTW, I would apply the same logic, only more so, to the changes to the CMakelists.txt. So long as cmake is still there, I would not want to break the cmake builds. Of course, when you're ready you could remove cmake completely. Along with "Building on macOS with cmake.txt".

In Building on macOS.txt https://github.com/pistacheio/pistache/pull/1197#discussion_r1550326473:

+Homebrew (also known as "brew") is required. If not already installed, +follow the Homebrew instructions to install:

  • In your browser: https://brew.sh/

  • +Then, install the necessary brew packages via terminal command line:

  • brew install meson
  • brew install cmake
  • brew install doxygen
  • brew install googletest
  • brew install openssl
  • brew install rapidjson
  • brew install howard-hinnant-date +(Note: you can skip cmake if you will only use meson; and you may be
  • able to skip howard-hinnant-date)

  • +Convenience shell scripts are provided to make the build. At terminal,

I'm not sure I'm a fan of these shell scripts. Do they really need to be committed here? I had a brief look at them, and while they look fine I personally handle build commands and build directories differently (I also often use muon instead of meson). I get that they are convenient for you, but probably not useful enough to the others to be committed upstream.

DG: It would be easy to remove them of course. Or reduce the number. I do think that making it really easy to do the build is worthwhile, and I'm not sure cutting and pasting the commands from README gets there. Also, since macOS won't have prepackaged binaries initially, making build easy seems especially important. Happy to discuss, can do whatever in the end.

In CMakeLists.txt https://github.com/pistacheio/pistache/pull/1197#discussion_r1550329072:

+ +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()

Thanks for these CMake enhancements, but really, we don't want to support CMake in the long run. Dropping them from this patch set will make the diff smaller, which is always nice :)

DG: See my comment above.

In README.md https://github.com/pistacheio/pistache/pull/1197#discussion_r1550333232:

@@ -198,6 +200,11 @@ To download the latest available release, clone the repository over GitHub. $ git clone https://github.com/pistacheio/pistache.git

+To build for macOS, you can follow the instructions in:

Dropping the CMake stuff and the custom scripts would make the build instructions the same for Linux and macOS, right? I mean, apart from installing a compiler and brew (which if you're using Pistache you've probably already figured out), Meson should be able to compile the project fetching the required dependencies from the Wrap files on macOS too, right?

DG: Interesting point, it does get much closer.

Remaining differences:

In include/pistache/client.h https://github.com/pistacheio/pistache/pull/1197#discussion_r1550334234:

@@ -180,6 +188,8 @@ namespace Pistache::Http::Experimental RequestBuilder& body(std::string&& val); RequestBuilder& timeout(std::chrono::milliseconds val);

+ +

Was this whitespace added by mistake?

DG: Removed.

In include/pistache/client.h https://github.com/pistacheio/pistache/pull/1197#discussion_r1550334502:

     std::shared_ptr<Aio::Reactor> reactor_;
     ConnectionPool pool;
     Aio::Reactor::Key transportKey;

     std::atomic<uint64_t> ioIndex;
  • using Lock = std::mutex;
  • using Guard = std::lock_guard;
  • // Note: queuesLock is declared before requestsQueues.This means that

⬇️ Suggested change

  • // Note: queuesLock is declared before requestsQueues.This means that
  • // Note: queuesLock is declared before requestsQueues. This means that

DG: Done.

In include/pistache/eventmeth.h https://github.com/pistacheio/pistache/pull/1197#discussion_r1550335586:

+// Event Method +// eventmeth.h + +#ifndef _EVENTMETHH +#define _EVENTMETHH + +/ ------------------------------------------------------------------------- / + +#ifdef PISTACHE_FORCE_LIBEVENT + +// Force libevent even for Linux +#define _USE_LIBEVENT 1 + +// _USE_LIBEVENT_LIKE_APPLE not only forces libevent, but even in Linux causes +// the code to be as similar as possible to the way it is for APPLE +// (e.g. whereever possible, even on Linux it uses solely OS calls that are

⬇️ Suggested change

-// (e.g. whereever possible, even on Linux it uses solely OS calls that are +// (e.g. wherever possible, even on Linux it uses solely OS calls that are

DG: Done.

In include/pistache/eventmeth.h https://github.com/pistacheio/pistache/pull/1197#discussion_r1550336331:

+// Note: libevent's event.h should NOT be included in this file, it is included +// only in the eventmeth CPP file. eventmeth.h users should not be using +// libevent directly, since eventmeth is our wrapper for libevent

Why?

DG: ...because they'd be breaking through an abstraction layer if they did. Of course, there's nothing to stop other code using libevent for its only purposes if it so wishes. It just shouldn't use livevent to access eventmeth's internals. I rewrote this comment to make it friendlier.

In include/pistache/eventmeth.h https://github.com/pistacheio/pistache/pull/1197#discussion_r1550339983:

+// A type wide enough to hold the output of "socket()" or "accept()". On +// Windows, this is an intptr_t; elsewhere, it is an int. +// Note: Mapped directly from evutil_socket_t in libevent's event2/util.h +#ifdef _WIN32 +#define em_socket_t intptr_t +#else +#define em_socket_t int +#endif

On Win32, why not using the SOCKET type instead? And... Wait a minute... Why are we talking about Win32?

DG: I believe I was thinking of both macOS and Windows when I wrote those lines. SOCKET is better, I changed it as you suggest.

I could take out mentions of Windows altogether for now, though FWIW _WIN32 does already appear in subprojects/cpp-httplib/httplib.h.

In include/pistache/eventmeth.h https://github.com/pistacheio/pistache/pull/1197#discussion_r1550344724:

+{

  • ifdef _USE_LIBEVENT

  • class EmEvent;
  • class EmEventFd;
  • class EmEventTmrFd;
  • using Fd = EmEvent *;
  • using FdConst = const EmEvent *;
  • using FdEventFd = EmEventFd *;
  • using FdEventFdConst = const EmEventFd *;
  • using FdEventTmrFd = EmEventTmrFd *;
  • using FdEventTmrFdConst = const EmEventTmrFd *;

  • define FD_EMPTY NULL

Is this define exposed to users? If yes, its name is a bit generic, and I fear it may clash with some user-defined macros. What about prefixing it with PISTACHE_?

[DG] A user would never need to use FD_EMPTY. In fact, a user would never need to use eventmeth directly at all, they would just use the existing pistache interfaces.

That said, a lot of pistache header files are exposed to users, almost independent of whether they are "really" part of the API.

And it seems there is code in the world that uses FD_EMPTY, in a 6502 compiler. See: https://www.google.com/search?q=%22FD_EMPTY%22+site%3Agithub.com&lr=&sca_esv=c2efddc75740eaea&sca_upv=1&as_qdr=all&ei=NN0NZqP3F63h0PEPhNG2iAg&ved=0ahUKEwij4cLkkKeFAxWtMDQIHYSoDYEQ4dUDCBA&uact=5&oq=%22FD_EMPTY%22+site%3Agithub.com&gs_lp=Egxnd3Mtd2l6LXNlcnAiGiJGRF9FTVBUWSIgc2l0ZTpnaXRodWIuY29tSIgoULoHWIIlcAF4AJABAJgBRaABiAGqAQEyuAEDyAEA-AEBmAIAoAIAmAMAiAYBkgcAoAda&sclient=gws-wiz-serp

To set minds at rest, I changed FD_EMPTY to PS_FD_EMPTY. The latter does not appear on github.

In include/pistache/meson.build https://github.com/pistacheio/pistache/pull/1197#discussion_r1550347056:

@@ -13,6 +13,7 @@ install_headers( 'cookie.h', 'description.h', 'endpoint.h',

  • 'eventmeth.h',

This file is indented with tabs

DG: That's true, but I did not make it so :-) It's already tab-indented in the build. I've left this unchanged for now.

In include/pistache/eventmeth.h https://github.com/pistacheio/pistache/pull/1197#discussion_r1550350594:

  • enum EventCtlAction
  • {
  • EvCtlAdd = 1,
  • EvCtlMod = 2, // rearm
  • EvCtlDel = 3
  • };

Using an enum class would probably be preferable: ⬇️ Suggested change

  • enum EventCtlAction
  • {
  • EvCtlAdd = 1,
  • EvCtlMod = 2, // rearm
  • EvCtlDel = 3
  • };
  • enum class EventCtlAction
  • {
  • Add = 1,
  • Mod = 2, // rearm
  • Del = 3
  • };

—

DG: Sure. Changed.

Reply to this email directly, view it on GitHub https://github.com/pistacheio/pistache/pull/1197#pullrequestreview-1977755702, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAFMA2YYB4ICLQSE2YPTPU3Y3RSGVAVCNFSM6AAAAABFPY574WVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTSNZXG42TKNZQGI . 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 @. @.>

dgreatwood commented 2 months ago

Hi Dennis -

I think reverting out the formatting updates in the macOSSupport branch might be a meaningful amount of work.

However, to your point, I have submitted a PR "clangConformance" which brings all nonconforming source files into line (and does nothing else). JFYI, I did not update subprojects/cpp-httplib/httplib.h; it is so differently formatted probably better just to ignore it I think.

The changed files are only: include/pistache/async.h include/pistache/cookie.h include/pistache/mailbox.h src/common/net.cc src/common/reactor.cc (Could have been worse).

If you or @Kip Warner @.***> would like to accept the clangConformance PR, I'll be happy to merge the updated master back into macOSSupport. If I understand correctly, that should allow you to look at macOSSupport without seeing the format updates.

I guess for other active PRs, you can have submitters update, or else you guys can do it directly if you think that's necessary.

Thanks!

On Thu, Apr 18, 2024 at 10:01 PM Dennis Jenkins @.***> wrote:

@.**** commented on this pull request.

On include/pistache/async.h https://github.com/pistacheio/pistache/pull/1197#discussion_r1571795252:

Ah. hmm. that's annoying. There is never a good time to exercise the following option, but we could run "clang-format" from HEAD, then rebase all active PRs against that. its a one-time pain, and ruins "blame". However, I am not actively contributing to pistachio at the moment, so I won't force others to keep their PRs "pure" (syntax or semantic, but not both concurrently). However, please do consider splitting your PR into two, one for reformatting (zero logic changes), and then one that makes logic changes.

— Reply to this email directly, view it on GitHub https://github.com/pistacheio/pistache/pull/1197#discussion_r1571795252, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAFMA24YWIG6MVDSNS3DDADY6CQJHAVCNFSM6AAAAABFPY574WVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDAMJQGUZTCMJYG4 . You are receiving this because you were assigned.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 @. @.>

dgreatwood commented 2 months ago

@Kip Warner @.***> , thanks for updating 'master' with the clangConformance PR. I have remerged and repushed the macOSSupport branch from master.

Perhaps there are better ways to view the diff, but at any rate if I do:

https://github.com/pistacheio/pistache/compare/master...dgreatwood:pistachefork:macosSupport I now see the diff between the two, no longer showing the clang-format changes which of course are now present in both.

Thanks again.

On Fri, Apr 19, 2024 at 9:57 AM Duncan Greatwood @.***> wrote:

Hi Dennis -

I think reverting out the formatting updates in the macOSSupport branch might be a meaningful amount of work.

However, to your point, I have submitted a PR "clangConformance" which brings all nonconforming source files into line (and does nothing else). JFYI, I did not update subprojects/cpp-httplib/httplib.h; it is so differently formatted probably better just to ignore it I think.

The changed files are only: include/pistache/async.h include/pistache/cookie.h include/pistache/mailbox.h src/common/net.cc src/common/reactor.cc (Could have been worse).

If you or @Kip Warner @.***> would like to accept the clangConformance PR, I'll be happy to merge the updated master back into macOSSupport. If I understand correctly, that should allow you to look at macOSSupport without seeing the format updates.

I guess for other active PRs, you can have submitters update, or else you guys can do it directly if you think that's necessary.

Thanks!

On Thu, Apr 18, 2024 at 10:01 PM Dennis Jenkins @.***> wrote:

@.**** commented on this pull request.

On include/pistache/async.h https://github.com/pistacheio/pistache/pull/1197#discussion_r1571795252 :

Ah. hmm. that's annoying. There is never a good time to exercise the following option, but we could run "clang-format" from HEAD, then rebase all active PRs against that. its a one-time pain, and ruins "blame". However, I am not actively contributing to pistachio at the moment, so I won't force others to keep their PRs "pure" (syntax or semantic, but not both concurrently). However, please do consider splitting your PR into two, one for reformatting (zero logic changes), and then one that makes logic changes.

— Reply to this email directly, view it on GitHub https://github.com/pistacheio/pistache/pull/1197#discussion_r1571795252, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAFMA24YWIG6MVDSNS3DDADY6CQJHAVCNFSM6AAAAABFPY574WVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDAMJQGUZTCMJYG4 . You are receiving this because you were assigned.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 @. @.>

dgreatwood commented 2 months ago

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 commented 2 months ago

@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.

dgreatwood commented 2 months ago

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 https://github.com/pistacheio/pistache/pull/1197#discussion_r1588594210:

@@ -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 https://github.com/pistacheio/pistache/pull/1197#discussion_r1588591203:

+ +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 https://github.com/pistacheio/pistache/pull/1197#pullrequestreview-2037217152, 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 @. @.>

Tachi107 commented 1 month ago

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!

dgreatwood commented 1 month ago

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 https://github.com/pistacheio/pistache/pull/1197#discussion_r1622213866:

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 https://github.com/pistacheio/pistache/pull/1197#pullrequestreview-2090507154, 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 @. @.>

Tachi107 commented 1 month ago

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.

dgreatwood commented 1 month ago

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 https://github.com/pistacheio/pistache/pull/1197#issuecomment-2143395697, 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 @. @.>

dgreatwood commented 1 month ago

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 https://github.com/pistacheio/pistache/pull/1197#issuecomment-2143395697, 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 @. @.>

kiplingw commented 1 week ago

@Tachi107, if you need more time please let @dgreatwood and I know. Otherwise if you're done reviewing I am ready to merge.

Tachi107 commented 1 week ago

Hi Kip, sorry for my delayed reply. I have looked at a significant portion of the changes, but had to pause because of university exams.

What I've looked at was OK, and I expect the rest of the patch set to be high quality too. My only thought were stylistic nitpicks, but we all have our preferences :)

I'll be busy until the end of July, so I don't think it makes sense for me to hold this any longer. If you'd like to merge, go ahead :D

Duncan, thanks again for your huge contribution. It'd been a pleasure to meet you on video call, too!