Re: wip/cliqz: Request for review

[Santhosh Raju <santhosh.raju%gmail.com@localhost>]

On Thu, Jan 17, 2019 at 3:34 AM Roland Illig <roland.illig%gmx.de@localhost> wrote:
>
> 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.
>

Agreed on the DISTFILES being kept in a separate DIST_SUBDIR. I set
DIST_SUBDIR=${PKGBASE} so that there are no other packages that may
accidentally use that sub-directory. The changes are in the latest wip
commit.

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

Moved the SITES.* and related variables under the DISTFILES section.
The changes are in the latest wip commit.

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

This was taken directly from the www/firefox Makefile[1] line 63, and
when commented out it fails with

ERROR: *** The above files still have references to the build directory.
ERROR:     This is possibly an error that should be fixed by unwrapping
ERROR:     the files or adding missing tools to the package makefile!
*** Error code 1

The same error happens in www/firefox too.

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

This was taken directly from the www/firefox Makefile[1] lines 35-37,
I am not sure why it was done in www/firefox but I have commented out
NOT_PAX_MPROTECT_SAFE for now in cliqz to check if anyone has issues
running it.

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

I see the issue with usage of bsdtar, what would be the suggested
solution for this?

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

Yes, it seems I forgot to add the "cd ${DESTDIR}${PREFIX}/lib", I have
added that in the latest wip commit.

> 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?
>

This was taken directly from the www/firefox Makefile[1] lines 97-99,
a symlink also would have worked, but I just followed the convention
in www/firefox package.

Would you suggest creating a symlink over the approach in www/firefox?

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

I have removed the "+" from "+=" in the latest wip commit.

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

Thank you for the review.

I ran a build + install and this seem to have gone fine, looking
forward to the suggestions on the unresolved issues mentioned above.

References

1. http://anonhg.netbsd.org/pkgsrc/file/trunk/www/firefox/Makefile

Regards
Santhosh