Re: wip/cliqz: Request for review

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

On Thu, Jan 17, 2019 at 10:22 PM Roland Illig <roland.illig%gmx.de@localhost> wrote:
>
> Am 17.01.2019 um 16:06 schrieb Santhosh Raju:
> > 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.
>
> Sorry for the typo I made above. I meant DIST_SUBDIR=${PKGNAME_NOREV}
> since it includes both the PKGBASE and the PKGVERSION. Chances are high
> that in a later version of the package you want to update the Firefox
> extensions, therefore it is common to use PKGNAME_NOREV.
>

Understood, in the latest wip commit I have replaced ${PKGBASE} with
${PKGNAME_NOREV}

> >> The CHECK_WRKREF_SKIP looks a bit strange to me.
> >
> > 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.
>
> My idea for this case was to edit buildinfo.html in-place and replace
> ${WRKDIR} with a placeholder.
>

Doing this will require me to use sed(1) in do-install phase, since
the file is extracted with bsdtar(1). Which might be an uglier
solution.

Do you have any suggestions on how to do the replace during the
do-install phase?

> >> 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?
>
> When you run "chown -R ${ROOT_USER}:${ROOT_GROUP}" after extracting the
> archive, everything should be fine. Or it could be REAL_ROOT_USER
> instead of ROOT_USER, I'm not sure. Therefore I asked about the
> difference between these variables in a separate mail.
>

I have added a chown(1) command after extraction is complete in the
do-install phase. This change should be there in the latest wip
commit.

I did a bit of checking with ROOT_USER vs REAL_ROOT_USER and it looks
like ROOT_USER is what I should be using.

When doing the check I found that ROOT_USER is the user name of the
user running the command, while REAL_ROOT_USER gave me root.

Regards
Santhosh