Re: wip/cliqz: Request for review

[Roland Illig <roland.illig%gmx.de@localhost>]

Am 16.01.2019 um 13:48 schrieb Santhosh Raju:
> Hello
> 
> I have a request for review for wip/cliqz package[1] in pkgsrc-wip git
> repository.
> 
> Cliqz[2] is a browser based on Mozilla Firefox, with customization to
> implement the cliqz specific search and privacy stuff.

Wow. This looks like a lot of work, given that Cliqz is based on Firefox
and that package usually needs lots of pkgsrc-specific patches.

I'm impressed that even pkglint -Wall doesn't find anything to complain
about your package, even though it contains many patches (as usual for
packages based on Firefox). Congratulations.

Since pkglint doesn't complain at all, I'll have to take a deeper breath
and can concentrate on the more complicated things.

Several of the DISTFILES don't have a version number in their filename,
and I expect that over time, these will receive updates from upstream,
especially adult-domains.bin. This will make your package unbuildable
because some day the checksums will not match anymore.

An immediate fix for this is to define DIST_SUBDIR=${PKGVERSION_NOREV}
(see bmake help topic=distfiles) so that other packages are not
negatively affected when these files are downloaded to the globally
shared DISTDIR.

As a reader of the package Makefile, I had expected the SITES.*
definitions much closer to the definition of DISTFILES. In the canonical
order of variable names, SITES.* comes at the end of the first
paragraph, right below DISTFILES. This avoids any confusion about
whether the Firefox extensions are downloaded with a specific version
number. See https://netbsd.org/docs/pkgsrc/pkgsrc.html#components.Makefile.

The CHECK_WRKREF_SKIP looks a bit strange to me. I don't see a point why
including this information in the generated binary makes any sense.
Especially since none of the other files are mentioned in
CHECK_WRKREF_SKIP. Therefore I'd rather remove that information from the
buildinfo.html file.

The NOT_PAX_MPROTECT_SAFE could need a bit more of an explanation. The
associated Git commit only says "Fixed build in netbsd/amd64". I'd like
to know what exactly the error messages were (if any) or what other
reason makes these lines in the Makefile necessary. An internet browser
written in C/C++ should definitely have all available security measures
enabled by default.

I don't like bsdtar to be used in the do-install target. Pkglint already
warns about using ${CP} and pax -pe in the *-install targets, but it
doesn't know about bsdtar yet. The problem is that when the package is
installed as root, the owner/group of the files is set to the
information from the archive. The owner/group of the files should always
be 0/0, or UNPRIVILEGED_USER for unprivileged pkgsrc builds.

In do-install, the "${FIND} ." searches in pkgsrc/wip/cliqz, which
sounds really strange. Did you forget a "cd ${DESTDIR}${PREFIX}/lib"? In
the same command line, the "-type f" and "-exec" lines should be
indented, just like the arguments to ${FIND} already are. If you had
defined WRKOBJDIR to be outside the pkgsrc directory (see bmake help
topic=work), you would have caught this bug earlier since then the work
directory is created as a symlink, and ${FIND} doesn't descend into it.

In post-install, to save system resources at runtime, the second command
should be ${ECHO} 'exec ${PREFIX}...' to reduce the number of running
processes. This also ensures that the exit code is passed correctly.
Alternatively, what about a simple symlink?

In options.mk, PKG_SUPPORTED_OPTIONS should be defined using the "="
operator instead of "+=". I'll add a pkglint warning for that.

That's all I could find. All in all, it looks pretty good to me.

Best,
Roland