Hi Maxime, many thanks for the review!

Maxime Devos <maximedevos@telenet.be> writes:

> Nicolò Balzarotti schreef op di 27-04-2021 om 15:56 [+0200]:
>> +    (synopsis "C++ header-only HTTP/HTTPS server and client library")
>> +    (description "cpp-httplib is a C++11 single-file header-only cross
>> +platform blocking HTTP/HTTPS library, easy to setup.  Just include the
>> +@file{httplib.h} file in your code!")
>
> This is a little misleading, as shared libraries are build, as BUILD_SHARED_LIBS
> is enabled.  Maybe "cpp-http is a single-file header-only library" -->
> "cpp-http can be used as a single-file header-only 
> library"?
>
I removed it from the synopsis, and reworded the description to: 1. make
it sound less like marketing, and keeping the single-header thing as a bonus.

> About ‘header-only’: this is true, but ultimately irrelevant to the user
> (= C++ developer on a Guix System or using Guix on top of a foreign distro).
> But there's also a desirable thing called ‘portability’, the user might be
> searching for a single-header web server software to distribute to other
> people (not on Guix) in source form ...
>
> I'm conflicted if "single-file header" should be included in the description.
> If you decide to remove it, I suggest you add a comment like
>
>   ;; this package is not graftable, as everything is implemented in a single
>   ;; header
>
> to prevent trouble in a (admittedly somewhat far-fetched, no insult intended
> to its developers) future where cpp-httplib becomes a very popular dependency
> in Guix.
>
I don't know enought about grafts, so I trust you on that.  I did not
know where to put the comment, so I added it at the very top.

I also noticed that this was updated since last time, so I updated it to
0.8.8 (latest tagged version).

>> +       #:phases
>> +       (modify-phases %standard-phases
>> +         (replace 'check
>> +           (lambda* (#:key source #:allow-other-keys)
>> +             ;; openssl genrsa wants to write a file in the git checkout
>> +             (copy-file (string-append source "/test") "test")
>> +             (chmod "test" #o744)
>> +             (invoke "make"))))))
>
> Tests most likely should not be run when cross-compiling.

> I'm not 100% sure, but you might need to do something like
>
>> +           (lambda* (#:key tests? source #:allow-other-keys)
>> +             ;; openssl genrsa wants to write a file in the git checkout
>> +             (when tests?
>> +               (copy-file
> (string-append source "/test") "test")
>> +               (chmod "test" #o744)
>> +               (invoke "make")))))))
>

Didn't think about that, I wrapped it in a `when tests?` (and added
`tests?` as argument to the lambda) as you suggested.  I also changed it
a bit making it more clear.  There are now tests requiring network
access, so I disabled them.

>
>> +       ("zlib" ,zlib)))
>
> In <https://github.com/yhirose/cpp-httplib/blob/master/httplib.h> I see
> a few lines
>
>   #ifdef CPPHTTPLIB_ZLIB_SUPPORT
>   #include <zlib.h>
>   #endif
>
> so it seems zlib should be in (inputs ...) instead.
>

> I also saw these lines:
>
>   #ifdef CPPHTTPLIB_BROTLI_SUPPORT
>   #include <brotli/decode.h>
>   #include <brotli/encode.h>
>   #endif
>
> Would it be useful to include brotli?
>
>   #ifdef CPPHTTPLIB_OPENSSL_SUPPORT
>   #include <openssl/err.h>
>   #include <openssl/md5.h>
>   ...
>
> Likewise, for openssl?
Sure, added brotli and moved openssl to inputs.  I also aadded the
"HTTPLIB_REQUIRE_" flags just to be sure they are used int the build.
They shouldn't be needed as HTTPLIB_USE_*_IF_AVAILABLE defaults to ON,
but if they change default we are covered.


Regarding why openssl and zlib were in native-inputs, this is (probably)
how it went: I built it without openssl, tests failed to run because the
command openssl was required to generate a certificate, so I added it to
native-inputs.  Then probably I added zlib not noticing I placed it
under native-inputs, at least that's how I think it went.

nheko still builds and run, so here the v5 of the series.

>
> Greetings,
> Maxime.

Thanks, Nicolò