Re: cmake/build.mk

[Roland Illig <roland.illig%gmx.de@localhost>]

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".

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.

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.

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'.

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.

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.

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

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.

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

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.

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').

Roland