tech-pkg archive

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]

Re: Bring BLAS into pkgsrc, pt. II



On 2019-07-23 12:44, Roland Illig wrote:
# $NetBSD$
#
# This Makefile fragment is meant to be included by packages
# that use any BLAS implementation instead of one particular one.
In this introductory paragraph, it would be nice to describe what BLAS
actually means. A simple "that use any BLAS (Basic Linear Algebra
Subprograms) implementation" already suffices.

# Since we always ship BLAS and LAPACK together (as upstream variants
Who is "we", and how does the reader know that?

# TODO: Also set a variable to find a matching pkg-config file
# to avoid duplicating its contents in BLAS_LIBS?
There should be as few TODO lines in pkgsrc/mk as possible. (I know I'm
guilty of adding several TODOs to pkgsrc myself as well, though.)

# === Per-package variables ===
This should rather be the established "Package-settable variables", as
in many other files. Someday pkglint will parse these descriptions and
base its checks on them. Therefore it's nice if the wording is
consistent. This also benefits the human readers.

Currently, "bmake help topic=blas" only says:

     No help found for blas, but it appears in:

     mk/blas.buildlink3.mk
     wip/mk/blas.buildlink3.mk

To change this, you can add a comment "# Keywords: blas lapack" at the
end of the introductory comment block.

.include "../../mk/bsd.prefs.mk"
This should simply be '.include "bsd.prefs.mk"', as bsd.prefs.mk will be
in the same directory as blas.buildlink3.mk.

.if(!empty(_BLAS_MATCH))
Usually we put a space between the .if and the condition. There's no
pkglint check for that yet since I am seeing this for the very first
time. Also the outer parentheses are redundant.

.if defined(_BLAS_PACKAGE)
.include "../../${_BLAS_PACKAGE}/buildlink3.mk"
.endif
This variable is closely related to the PKGPATH variable. Since the name
"PACKAGE" is highly ambiguous (it could mean one of PKGNAME,
PKGNAME_NOREV, PKGPATH, ../../PKGPATH, basename(PKGPATH)), I would
prefer this variable to be called _BLAS_PKGPATH instead.

This variable name probably originated from _MPI_PACKAGE, so I see it's
picking up existing practice. Apart from that, the word PACKAGE is not
used in variable names to identify a package, probably because of this
ambiguity.


All these issues have been addressed, I beefed up the comments to provide some background and guidance for BLAS newbies, and Thomas and I worked out a couple other minor issues.

At this point I'd like to request clearance to commit (with minor path changes as it is moved from wip), or ask someone else to do the commit for us, assuming nobody sees any remaining issues.

Note that this commit should not affect any existing packages.  It's a new mk file that nothing uses yet.

I have tested all existing BLAS dependents against it under wip/mk and will update and retest them after it's committed.

Is there anything special about a commit to mk vs. a package commit?  I have never committed to mk before.



Home | Main Index | Thread Index | Old Index