Re: NGINX Unit pkgsrcs

["Sergey A. Osokin" <osa%freebsd.org@localhost>]

Thanks for the review, Roland.

On Wed, Nov 18, 2020 at 12:58:51AM +0100, Roland Illig wrote:
> On 17.11.2020 23:11, Sergey A. Osokin wrote:
> > Hi there,
> >
> > I've created several pkgsrcs in WIP, so I'm looking for a way to review those.
> > The pkgsrcs I've created are for NGINX Unit, https://unit.nginx.org/, a poliglot
> > application server, a reverse proxy and a static file server.
> 
> Just a bit of terminology.  What you created were not pkgsrcs but simply
> packages.  The word pkgsrc is only used for the system as a whole.

Got it, thanks!

> > So, here is the list of the pksrcs for review (all of those are in wip.pkgsrc.org):
> > www/unit
> > www/unit-perl
> > www/unit-php
> > www/unit-python
> > www/unit-ruby
> 
> I had a look at them, and they look really well-done and nontrivial,
> involving rc.d scripts and so on.
> 
> I don't understand unit/files/patch-auto-modules-php.  The deleted and
> the added line look exactly the same.

The patch adds a comma symbol right of the "-rpath":
-                             -Wl,-rpath ${NXT_PHP_LIB_PATH}"
+                             -Wl,-rpath,${NXT_PHP_LIB_PATH}"

The main reason for the patch is NGINX Unit's PHP module can't be
built without that one with the following error:

cd /pkgsrc/www/unit-php/work/unit-1.20.0 &&  ./configure php --lib-path=/usr/pkg/lib  --module=php5.6.40
configuring PHP module
checking for PHP ... found
 + PHP SAPI: [cli embed cgi]
checking for PHP version ... not found
checking for PHP embed SAPI ... not found

./configure: error: no PHP embed SAPI found.

*** Error code 1

Here's the part of the code needs to be built:
----------------------------------------
checking for PHP embed SAPI
gcc: Missing argument for -Wl,-rpath
----------

    #include <php.h>
    #include <php_main.h>

    int main() {
        php_module_startup(NULL, NULL, 0);
        return 0;
    }
----------
gcc -O2 -D_FORTIFY_SOURCE=2 -I/usr/pkg/include -I/usr/include -pipe -fPIC -fvisibility=hidden -O -W -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wmissing-prototypes -Werror -g -I/usr/pkg/include/php -I/usr/pkg/include/php/main -I/usr/pkg/include/php/TSRM -I/usr/pkg/include/php/Zend -I/usr/pkg/include/php/ext -I/usr/pkg/include/php/ext/date/lib -o build/autotest build/autotest.c -lphp5 -L/usr/pkg/lib -Wl,-rpath /usr/pkg/lib -L/usr/pkg/lib

> That's something pkglint doesn't
> check for, since it doesn't expect something like this to ever appear.
> And in the last 15 years it indeed never happened.
> 
> Is the patch intended to be fixed upstream? Then you should mention the
> bug report in the patch, above the "---".

The issue's been reported to the upstream informally and will be fixed
with the next release.

> Instead of unitversion.mk, you can just call that file version.mk.
> Repeating the word "unit" is not necessary.

Done.

> Running pkglint is a good idea.  And if pkglint doesn't complain
> anymore, try pkglint -Wall.  It keeps some surprises for you.

Will do.

> All in all, the packages look very good already.

Thanks.

-- 
Sergey A. Osokin