Re: cmake/build.mk

[Thomas Klausner <wiz%NetBSD.org@localhost>]

Hi Roland!

Thank you for the detailed review!

On Tue, Jul 26, 2022 at 11:50:23PM +0200, Roland Illig wrote:
> Am 26.07.2022 um 22:16 schrieb Thomas Klausner:
> > Hi!
> > 
> > Following a discussion we had earlier about improving cmake support,
> > I've created a devel/cmake/build.mk (to match devel/meson/build.mk).
> > 
> > Comments?
> 
> Looks simple and straight-forward to me. A few remarks:
> 
> CMAKE_USE_NINJA treats the mixed-case "Yes" as "no". I like the strict
> lowercase value set, but it's something new for pkgsrc. Traditionally,
> "Yes" means the same as "yes" and "yES".

I've added the "tl" modifier so all variants of "yes" will be treated
the same.

> At which point of a package Makefile is devel/cmake/build.mk intended to
> be included? It should be relatively late, at a point where the values
> of BUILD_DIRS, INSTALL_DIRS and TEST_DIRS no longer change. Probably
> directly above bsd.pkg.mk.

I thought in the bl3.mk block at the bottom.

> The quotes around "Unix Makefiles" should not be there. Instead,
> wherever CMAKE_BUILD_SYSTEM is used, it should rather be used in the
> form ${CMAKE_BUILD_SYSTEM:Q}, quoting the string only where necessary.

Fixed.

> TEST_DIRS should be defined between BUILD_DIRS and INSTALL_DIRS, to
> reflect the usual order of the phases. Same for all other occurrences of
> 'test' or 'TEST'.

Moved.

> The directories in CONFIGURE_DIRS and the other variables must be
> relative to WRKSRC. Contrary to CONFIGURE_DIRS in other scenarios,
> specifying absolute paths does not work here. This is new in pkgsrc but
> shouldn't be a problem for any package.

I documented this now it as relative to WRKSRC. (I didn't know
absolute paths worked!?)

> The variable CMAKE_BUILD_ARGS is only defined in the Ninja case, but not
> in the Unix Makefiles case. On the other hand, CMAKE_INSTALL_ARGS
> explicitly defaults to an empty string. This is asymmetric.

symmetry++

> The shell command lines should be prefixed with ${RUN}, as the value of
> ${MAKE_ENV} is too verbose to be output by default.

Ok.

> The line wrapping differs between cmake-build and cmake-test. In both
> these targets, ${CMAKE_BUILD_TOOL} should be at the beginning of a
> visual line, even if that means to use 3 lines for the whole command
> line. In cmake-configure, ${SETENV} should also be at the beginning of a
> visual line.

Done.

> In cmake-configure, the environment should only be CONFIGURE_ENV, not
> MAKE_ENV. Unless it's intentional, in which case it should be documented.

No, it wasn't. Fixed.

> I don't see why including bsd.prefs.mk is necessary at all. The only
> variables that are accessed at load time are the directory lists, and
> these should be lists of literal strings.

Removed.

> When the API has stabilized, the file should get the usual documentation
> header at the top (for 'make help') and the _VARGROUPS section at the
> bottom (for 'make show-all-cmake').

I don't know anything about _VARGROUPS and didn't find any
documentation. Can you please tell me more about this and/or document
it?

New version attached.

Thanks,
 Thomas

# $NetBSD$
#
# This Makefile fragment supports building using the CMake build tool.
#
# User-settable variables:
#
# CMAKE_USE_NINJA
#	Build using the ninja tool.
#
#	Possible: yes no
#	Default: no
#
# Package-settable variables:
#
# CMAKE_CONFIGURE_ARGS
#	Arguments to pass to CMake during the configure stage. Defaults
#	to CMAKE_ARGS for backwards compatibility with USE_CMAKE.
#
# CMAKE_BUILD_ARGS
#	Arguments to pass to CMake during build. Default: empty
#
# CMAKE_INSTALL_ARGS
#	Arguments to pass to CMake during installation: Default: empty
#
# CONFIGURE_DIRS
#	Directories relative to WRKSRC in which to run CMake. Usually
#	only one, the toplevel.
#
# BUILD_DIRS
#	Directories relative to WRKSRC in which to build. Defaults
#	to CONFIGURE_DIRS.
#
# INSTALL_DIRS
#	Directories relative to WRKSRC in which to run the 'install'
#	step. Defaults to CONFIGURE_DIRS.
#
# TEST_DIRS
#	Directories relative to WRKSRC in which to run the tests. Defaults
#	to CONFIGURE_DIRS.

CMAKE_REQD?=	0
.for version in ${CMAKE_REQD}
TOOL_DEPENDS+=	cmake>=${version}:../../devel/cmake
.endfor

CMAKE_CONFIGURE_ARGS?=	${CMAKE_ARGS}

CMAKE_BUILD_DIR?=	cmake-pkgsrc-build
CMAKE_USE_NINJA?=	no
.if ${CMAKE_USE_NINJA:tl} == "yes"
TOOL_DEPENDS+=		ninja-build-[0-9]*:../../devel/ninja-build
CMAKE_BUILD_SYSTEM?=	Ninja
CMAKE_BUILD_TOOL?=	ninja
CMAKE_BUILD_ARGS?=	-j ${_MAKE_JOBS_N:U1}
CMAKE_INSTALL_ARGS?=	# empty
.else
CMAKE_BUILD_SYSTEM?=	Unix Makefiles
CMAKE_BUILD_TOOL?=	${MAKE}
CMAKE_BUILD_ARGS?=	# empty
CMAKE_INSTALL_ARGS?=	# empty
.endif

CONFIGURE_DIRS?=	.
BUILD_DIRS?=		${CONFIGURE_DIRS}
INSTALL_DIRS?=		${CONFIGURE_DIRS}
TEST_DIRS?=		${CONFIGURE_DIRS}

.PHONY: cmake-configure cmake-build cmake-install cmake-test

do-configure: cmake-configure
cmake-configure:
.for d in ${CONFIGURE_DIRS}
	${RUN} cd ${WRKSRC}/${d} && ${SETENV} ${CONFIGURE_ENV} cmake \
		--install-prefix ${PREFIX} \
		-B ${CMAKE_BUILD_DIR} \
		-G ${CMAKE_BUILD_SYSTEM:Q} \
		${CMAKE_ARGS}
.endfor

do-build: cmake-build
cmake-build:
.for d in ${BUILD_DIRS}
	${RUN} cd ${WRKSRC}/${d}/${CMAKE_BUILD_DIR} && \
		${SETENV} ${MAKE_ENV} \
		${CMAKE_BUILD_TOOL} ${CMAKE_BUILD_ARGS}
.endfor

do-test: cmake-test
cmake-test:
.for d in ${TEST_DIRS}
	${RUN} cd ${WRKSRC}/${d}/${CMAKE_BUILD_DIR} && \
		${SETENV} ${TEST_ENV} \
		${CMAKE_BUILD_TOOL} ${CMAKE_BUILD_ARGS} test
.endfor

do-install: cmake-install
cmake-install:
.for d in ${INSTALL_DIRS}
	${RUN} cd ${WRKSRC}/${d}/${CMAKE_BUILD_DIR} && \
		${SETENV} ${INSTALL_ENV} \
		${CMAKE_BUILD_TOOL} ${CMAKE_INSTALL_ARGS} install
.endfor