On Mon, Mar 02, 2020 at 05:57:26PM +0000, Stuart Henderson wrote:
> > Regarding Makefile.inc:
> > --8<--
> > ...
> > NO_TEST = Yes
> >
> > do-install:
> > ${INSTALL_DATA_DIR} ${PREFIX}/share/examples/dbip
> > ${INSTALL_DATA} ${WRKDIST}/*.mmdb ${PREFIX}/share/examples/dbip
> > chown -R ${SHAREOWN}:${SHAREGRP} ${PREFIX}/share/examples/dbip
> > -->8--
> >
> > I would expect a Makefile.inc to set default values that can be
> > overriden from underlying ports, a la:
> >
> > --8<--
> > NO_TEST ?= Yes
> >
> > .if !target(do-install)
> > do-install:
> > ${INSTALL_DATA_DIR} ${PREFIX}/share/examples/dbip
> > ${INSTALL_DATA} ${WRKDIST}/*.mmdb ${PREFIX}/share/examples/dbip
> > chown -R ${SHAREOWN}:${SHAREGRP} ${PREFIX}/share/examples/dbip
> > .endif
> > -->8--
>
> It's useful sometimes where there's more divergence between the ports
> in subdirectories, but that does seems a bit overengineered for this,
> especially the ".if !target".
I have no strong opinion about this part, happy to make the suggested
changes if we want to go this way.
> > Regarding the PLISTs,
> > --8<--
> > @comment $OpenBSD: PLIST,v$
> > share/examples/dbip/
> > @sample ${LOCALSTATEDIR}/db/dbip/
> > share/examples/dbip/dbip-city-lite.mmdb
> > @sample ${LOCALSTATEDIR}/db/dbip/dbip-city-lite.mmdb
> > -->8--
> >
> > Why use @sample here? Is that the way the old geoip database port
> > worked? I'm not strongly objecting but I'm not sure there's a point in
> > providing a user-editable file when there's no easy way to edit and
> > update it.
>
> Agreed, in the case of old geoip1 the filenames were pretty much fixed and
> the @sample method made sense (plus you might run an update tool to fetch
> new files in place of the old ones). For maxminddb it seems standard to
> configure the filename instead and if users are updating separately it
> would seem sensible to use different files for it, so I think it would
> be better to skip the @sample and just set PREFIX instead so they are
> installed directly to ${LOCALSTATEDIR}/db/dbip..
Makes sense, thanks for providing the details and rationale.
Here is a new revision to address the PLIST and PREFIX parts.
dbip.tar.gz
Description: application/gzip
