Page MenuHomeFreeBSD

Make ninja opt-out in cmake.mk.
ClosedPublic

Authored by tcberner on May 15 2017, 7:47 PM.
Tags
None
Referenced Files
F79490701: D10748.id29196.diff
Tue, Mar 19, 2:46 AM
Unknown Object (File)
Mon, Mar 4, 2:55 AM
Unknown Object (File)
Feb 17 2024, 9:23 PM
Unknown Object (File)
Feb 17 2024, 9:23 PM
Unknown Object (File)
Feb 17 2024, 9:23 PM
Unknown Object (File)
Feb 17 2024, 9:23 PM
Unknown Object (File)
Feb 17 2024, 9:23 PM
Unknown Object (File)
Feb 17 2024, 9:23 PM

Details

Summary

As kwm@ reported a quite significant build-speedup in wekbit2-gtk3 when using ninja instead of gmake, I'm interested to know whether using the ninja-generator of cmake could lead to at least some amount of speedup in other ports as well -- as cmake is used by many ports, this could be significant for the package builders.

At the moment cmake.mk has an opt-in to use ninja. Make this an opt-out.

Test Plan
  • Exp-run to see what breaks?
  • Pick some ports and compare build times?

Diff Detail

Repository
rP FreeBSD ports repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Yes this breaks some ports.

How many? ^^

As there are apparently some ports breaking with ninja, make disabling ninja an argument to cmake.mk noninja instead of a variable. This way the broken ports can be fixed cleanly by just setting USES=cmake:<otherargs>,noninja.

I will test-build the ports depending on ninja... to see what breaks.

  • Add upstream fix for cmake/ninja to graphics/ilmbase.

Don't use ninja for databases/mysql56-server:

ninja: error: dependency cycle: sql/GenServerSource -> sql/CMakeFiles/GenServerSource -> sql/sql_builtin.cc -> cmake_order_depends_target_sql -> sql/GenDigestServerSource -> sql/CMakeFiles/GenDigestServerSource -> sql/lex_token.h -> sql/gen_lex_token -> sql/GenServerSource

Maybe we could call noninja just "make"?

USES=cmake:outsource,make
# make                use make instead of ninja

Maybe we could call noninja just "make"?

USES=cmake:outsource,make
# make                use make instead of ninja

cmake runs cmake, not make. noninja is clearer.

Mark some more ports to not use ninja -- still ~500 more ports to go.

Ok, I think I should have gotten more-or-less all...

5-6% of the affected ports needed noninja.

Ping portmgr -- what's the opinion on this?

Regenearte against current ports-tree for exp-run.

Fix

  • mysql57-server
  • mysql80-server
  • gemrb
  • kde-workspace

I just realized that this change doesn't have a ports/CHANGES yet, could you prepare one?

Like describe this change and how people can opt-out if ninja isn't supported.

Sorry for chiming in so late.

I think this diff also obsoletes https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=213331 in the sense that we'd no longer have USES=cmake together with USES=gmake (only USES=cmake:noninja gmake, which is fine).

To complement what @kwm said, this patch is lacking documentation: right now, I don't think it's clear to other ports contributors when to add noninja at all. I tested a handful of the ports that need noninja here on Poudriere and they seem to fall into two categories:

  • Broken CMake files that have broken dependency rules that confuse ninja.
  • Ports setting BUILD_WRKSRC to build only part of the source code. Since build.ninja is generated in the top-level work directory, calling ninja from a subdirectory fails.

I wonder if it makes sense to try to fix the second case automatically in Uses/cmake.mk.

  • Fix x11-themes/kde4-style-bespin
  • add CHANGES as requested by kwm@

I wonder if it makes sense to try to fix the second case automatically in Uses/cmake.mk.

Sure, it's worth a try

I could not consitently reproduce the build failures of

  • devel/doxygen
  • math/stp

on i386. So make them not use ninja (for now).

Further drop the USES=gmake from

  • graphics/osg-devel

which was added for FreeBSD 10.x

Additionally, mark ports ignored that set USES=cmake gmake.

I've added some inline comments about ports that do not need noninja. In addition to what you added to CHANGES, I'd expect some documentation in Uses/cmake.mk as well, otherwise it's not much better than not adding anything at all. Specifically, there needs to be some documentation explaining that noninja is a stop-gap measure that's normally used by:

  • meta-ports (or ports that change BUILD_WRKSRC)
  • Ports with broken CMake rules (ninja refuses to work with circular dependencies, multiple rules that generate the same files or custom targets that do not quote $ symbols properly).
  • Ports that call CMake themselves as part of the build, as we end up with something that uses make and ninja at different points.
comms/uhd/Makefile
29 ↗(On Diff #29196)

I've fixed this in rP442624 so you can drop the noninja here.

databases/clickhouse/Makefile
37 ↗(On Diff #29196)

It worked fine here without noninja, is it really needed?

databases/mariadb100-server/Makefile
32 ↗(On Diff #29196)

This one also worked fine without noninja here.

databases/soci/Makefile
16 ↗(On Diff #29196)

This too. It's broken in -HEAD with or without the change.

devel/bullet/Makefile
19 ↗(On Diff #29196)

You can drop noninja if you pass -DINSTALL_LIBS:BOOL=ON to CMake.

devel/compiler-rt/Makefile
22 ↗(On Diff #29196)

This one works if you stop setting MAKE_ARGS.

devel/rapidjson/Makefile
26 ↗(On Diff #29196)

This one works if you drop MAKE_ARGS (which is superfluous anyway).

devel/xxhash/Makefile
18 ↗(On Diff #29196)
games/gemrb/Makefile
27 ↗(On Diff #29196)

Fixed in rP442686.

games/spring/Makefile
35 ↗(On Diff #29196)

Fixed in rP442691.

graphics/cuneiform/Makefile
18 ↗(On Diff #29196)

This port is just broken on HEAD, it builds fine with ninja on 10.3-i386.

graphics/opencv/Makefile
17 ↗(On Diff #29196)

The port built fine here without it.

graphics/partio/Makefile
13 ↗(On Diff #29196)

Fixed in rP442692.

lang/sagittarius-scheme/Makefile
23 ↗(On Diff #29196)

Fixed in rP442750.

net/libproxy/Makefile
16 ↗(On Diff #29196)

This port built fine without noninja here on HEAD.

www/wt/Makefile
25 ↗(On Diff #29196)

This port built fine on HEAD without noninja here.

I have trouble figuring out if there are more ports using cmake that should use ninja that ports that should not use ninja. And so, I am wondering if it should not be a USES=cmake:ninja to enable it.

Mk/Uses/cmake.mk
104 ↗(On Diff #29196)

remove empty line.

In D10748#230050, @mat wrote:

I have trouble figuring out if there are more ports using cmake that should use ninja that ports that should not use ninja. And so, I am wondering if it should not be a USES=cmake:ninja to enable it.

Using freshports.org/devel/cmake as reference and filtering out ports that no longer exist, there are 1317 ports that have a build-time dependency on CMake, ~70 of which don't work with ninja as far as I can see.

Mmmm, ok, good for me :-)

Next question, do we really a need a USES=ninja, so, are there ports that use USES=ninja but not USES=cmake (and that could not use USES=cmake) ?

In D10748#230070, @mat wrote:

Mmmm, ok, good for me :-)

Next question, do we really a need a USES=ninja, so, are there ports that use USES=ninja but not USES=cmake (and that could not use USES=cmake) ?

Ports using meson for instance.

Revert setting noninja for the ports mentioned by @rakuco.

Make the patch to devel/xxhash apply.

Drop USES=gmake from comms/uhd lang/sagittarius-scheme, now that they don't set noninja, they need to have gmake dropped.

tcberner marked 5 inline comments as done.
  • Don't use ninja, if the BUILD_WRKSRC or INSTALL_WRKSRC is different from CONFIGURE_WRKSRC
  • Fix devel/bullet by adding the mentioned CMAKE_ARGS.
  • Add a bit more comment to the noninja option.
  • Remove some noninja's that are now handled by the _WRKSRC work around

Readd noninja to databases/mariadb100-server

Remove ninja from the remaining evolution ports.

  • Remove CMAKE_NINJA from graphics/rawtherapee
  • Add CMAKE_NINJA to SANITY_DEPRECATED in bsd.sanity.mk

Minor cleanup: remove ninja from ports having USES=cmake.

Mk/bsd.sanity.mk
180 ↗(On Diff #29802)

I think it should go to SANITY_NOTNEEDED instead.

Put CMAKE_NINJA into SANITY_NOTNEEDED instead.

Ping portmgr and kde -- is this good to go now?

lgtm from my side, thanks for working on this. In your commit message, make sure you mention why some changes were done (e.g. why MAKE_ARGS was removed from compiler-rt).

This revision was automatically updated to reflect the committed changes.