Re: mailgraph package

[Frédéric Fauberteau <triaxx%NetBSD.org@localhost>]

Le 2019-10-01 22:00, Roland Illig a écrit :
> Am 01.10.2019 um 11:48 schrieb Frédéric Fauberteau:
> > # $NetBSD: Makefile,v 1.25 2019/08/11 13:21:29 wiz Exp $
> >
> > DISTNAME=    mailgraph-1.14
> > PKGREVISION=    6
> > CATEGORIES=    mail
>
> Somehow your mail client replaced the tabs with spaces. If that's also
> in the original Makefile, just run pkglint -F.

It's either my mail client or gedit that replaced the tabs. pkglint -F 
is silent and the Makefile is ok.

> > MASTER_SITES=    http://mailgraph.schweikert.ch/pub/
> > MASTER_SITES+=    http://mailgraph.schweikert.ch/pub/old
>
> I wonder why pkglint didn't complain about a MASTER_SITES that is
> missing the trailing slash. It should definitely not end with a letter
> since that would produce /pub/oldmailgraph-1.14.tar.gz without an
> intermediate slash.
>
> Ah I see, the "must end with a slash" check is only there for
> MASTER_SITES in the form of ${MASTER_SITE_APACHE:=subdir/}.

Ok, but I can add a slash at the end for security.

> There's an automatic https redirect on the host. Therefore you should
> change the URLs to use https as well. This is something that pkglint
> doesn't check because it never accesses the internet.

Done.

> > MAINTAINER=    kim%tac.nyc.ny.us@localhost
> > HOMEPAGE=    http://mailgraph.schweikert.ch/
> > COMMENT=    RRDtool frontend for mail statistics
> > LICENSE=    gnu-gpl-v2
> >
> > DEPENDS+=    rrdtool>=1:../../databases/rrdtool
> > DEPENDS+=    p5-File-Tail-[0-9]*:../../misc/p5-File-Tail
> >
> > USE_TOOLS+=    perl:run
> > NO_BUILD=    yes
> >
> > REPLACE_PERL+=    mailgraph.cgi mailgraph.pl
> >
> > BUILD_DEFS+=    VARBASE
> >
> > OWN_DIRS+=    ${VARBASE}/db/mailgraph
> >
> > RCD_SCRIPTS=    mailgraph
> >
> > SUBST_CLASSES+=        var
> > SUBST_STAGE.var=    pre-configure
> > SUBST_FILES.var=    mailgraph.cgi mailgraph.pl
> > SUBST_VARS.var=        VARBASE
> > SUBST_MESSAGE.var=    Fixing hardcoded /var.
>
> I prefer to put SUBST_MESSAGE further up, so that it serves as a 
> summary
> of the whole block. The documentation in mk/subst.mk mentions it after
> SUBST_STAGE. The order there is fully intentional and is strictly
> chronological. First, SUBST_STAGE determines the stage in which the
> substitution happens, and then SUBST_MESSAGE is shown. That's also the
> order that packages should follow.

Done. Thanks for the clear explanation.

> > AUTO_MKDIRS=        yes
> > INSTALLATION_DIRS+=    libexec/cgi-bin
> > INSTALLATION_DIRS+=    sbin
> > INSTALLATION_DIRS+=    share/doc/mailgraph
> > INSTALLATION_DIRS+=    share/mailgraph
>
> The first 3 INSTALLATION_DIRS lines are redundant since these
> directories are mentioned in the PLIST and AUTO_MKDIRS is defined.
> There's no pkglint check for this yet, I'll add one.

Should we prefer AUTO_MKDIRS instead INSTALLATION_DIRS?

> The last INSTALLATION_DIRS line is probably unnecessary. The package
> doesn't install anything there, and empty directories are not packaged.

mailgraph.css is installed in share/mailgraph:

  ${INSTALL_DATA} ${WRKSRC}/mailgraph.css \
      ${DESTDIR}${PREFIX}/share/mailgraph/mailgraph.css

But since AUTO_MKDIRS and directory in PLIST, no INSTALLATION_DIRS.

Thanks a lot for this very detailed review!