Page MenuHomeFreeBSD

Make ninja opt-out in cmake.mk.
ClosedPublic

Authored by tcberner on May 15 2017, 7:47 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
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; 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.

Yes this breaks some ports.

How many? ^^

tcberner updated this revision to Diff 28376.May 15 2017, 8:09 PM

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.

tcberner updated this revision to Diff 28380.May 15 2017, 8:43 PM
  • Add upstream fix for cmake/ninja to graphics/ilmbase.
tcberner updated this revision to Diff 28381.May 15 2017, 9:03 PM

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
lifanov added a subscriber: lifanov.EditedMay 16 2017, 5:03 PM

Maybe we could call noninja just "make"?

USES=cmake:outsource,make
# make                use make instead of ninja
adamw added a subscriber: adamw.May 16 2017, 5:07 PM

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.

tcberner updated this revision to Diff 28424.May 16 2017, 5:27 PM

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

tcberner updated this revision to Diff 28480.May 17 2017, 7:31 PM

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

5-6% of the affected ports needed noninja.

tcberner added a comment.EditedMay 29 2017, 8:52 AM

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

tcberner updated this revision to Diff 28968.May 29 2017, 9:07 AM

Regenearte against current ports-tree for exp-run.

tcberner updated this revision to Diff 29022.May 30 2017, 4:38 PM

Fix

  • mysql57-server
  • mysql80-server
  • gemrb
  • kde-workspace
tcberner updated this revision to Diff 29026.May 30 2017, 4:45 PM

Fix

  • devel/flang
kwm edited edge metadata.May 31 2017, 8:38 AM

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.

rakuco edited edge metadata.Jun 1 2017, 3:14 PM

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.

tcberner updated this revision to Diff 29110.Jun 1 2017, 4:44 PM
  • 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

tcberner updated this revision to Diff 29196.Jun 4 2017, 8:20 PM

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.

mat edited edge metadata.Jun 9 2017, 1:00 PM

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.

rakuco added a comment.Jun 9 2017, 1:04 PM
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.

mat added a comment.Jun 9 2017, 1:20 PM

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.

tcberner updated this revision to Diff 29437.Jun 10 2017, 6:18 PM

Revert setting noninja for the ports mentioned by @rakuco.

tcberner updated this revision to Diff 29438.Jun 10 2017, 7:19 PM

Make the patch to devel/xxhash apply.

tcberner updated this revision to Diff 29439.Jun 10 2017, 7:24 PM

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.Jun 12 2017, 2:55 PM
tcberner updated this revision to Diff 29490.
  • 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.
tcberner updated this revision to Diff 29492.Jun 12 2017, 3:55 PM
  • Add a bit more comment to the noninja option.
  • Remove some noninja's that are now handled by the _WRKSRC work around
tcberner updated this revision to Diff 29702.Jun 16 2017, 5:32 AM

Readd noninja to databases/mariadb100-server

tcberner updated this revision to Diff 29719.Jun 16 2017, 2:35 PM

Remove ninja from the remaining evolution ports.

tcberner updated this revision to Diff 29801.Jun 18 2017, 9:06 PM
  • Remove CMAKE_NINJA from graphics/rawtherapee
  • Add CMAKE_NINJA to SANITY_DEPRECATED in bsd.sanity.mk
tcberner updated this revision to Diff 29802.Jun 18 2017, 9:23 PM

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

mat added inline comments.Jun 19 2017, 1:18 PM
Mk/bsd.sanity.mk
180 ↗(On Diff #29802)

I think it should go to SANITY_NOTNEEDED instead.

tcberner updated this revision to Diff 29813.Jun 19 2017, 3:29 PM

Put CMAKE_NINJA into SANITY_NOTNEEDED instead.

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

rakuco accepted this revision.Jun 25 2017, 11:11 AM

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.