Bug 328695

Summary:	[PATCH] Ninja builder is hardwired to use "ninja" executable
Product:	[Applications] kdevelop	Reporter:	Andrew Stitcher <astitcher>
Component:	Build tools: Ninja	Assignee:	kdevelop-bugs-null
Status:	RESOLVED FIXED
Severity:	major	CC:	aleixpol, intelfx
Priority:	NOR
Version:	4.6.0
Target Milestone:	4.6.1
Platform:	Fedora RPMs
OS:	Linux
Latest Commit:	http://commits.kde.org/kdevelop/384b2335efc3c69a61914bf3c2874c492e1bbc0f	Version Fixed In:
Attachments:	Patch which fixes the problem for me Somewhat improved patch

Description Andrew Stitcher 2013-12-12 07:12:47 UTC

On Fedora (and maybe on Debian too) because of a name clash the ninja executable is named "ninja-build" however the ninja plugin is hardwired to use an executable called "ninja" and won't even load the plugin if it can't find this executable.

I think the correct behaviour is to look for both "ninja-build" and "ninja" using "ninja-build" if both are found (because of the name clash I mentioned)



Reproducible: Always

Steps to Reproduce:
1. Start up kdevelop with ninja plugin on Fedora 19 with ninja-build package installed.

Actual Results:  
1. Warning message is seen on console:
kdevelop(29851)/kdevplatform (shell) KDevelop::PluginController::loadPluginInternal: "Plugin 'Ninja Project Builder' could not be loaded correctly and was disabled.
Reason: ." 
kdevelop(29851) KDevNinjaBuilderPlugin::KDevNinjaBuilderPlugin: Ninja plugin installed but ninja is not installed. 
2. Plugin is not listed in list in help menu.

Expected Results:  
Plugin should be loaded. listed and able to be used

I have a patch which fixed the issue for me - I will attach here if I can.

Comment 1 Andrew Stitcher 2013-12-12 07:19:55 UTC

Created attachment 84055 [details]
Patch which fixes the problem for me

This is about the simplest solution I could think of, however it is not very flexible in that it just expands the possible names to "ninja" & "ninja-build" which should work for usual ninja installations I think, but isn't future proof in case it is called something different somewhere else.

Comment 2 Andrew Stitcher 2013-12-12 07:23:18 UTC

Another comment I'll make is that when the ninja plugin isn't loaded, but the cmake builder tries to build a ninja project it will just not do anything, and the build needs to be manually stopped.

It looks like the cmake builder will create a job for the ninja plugin even if it is not loaded which seems like another bug to me.

Comment 3 Andrew Stitcher 2013-12-12 07:50:25 UTC

Created attachment 84056 [details]
Somewhat improved patch

Tidied up patch a bit.

Comment 4 Kevin Funk 2013-12-12 08:32:27 UTC

Patch looks fine, but i'd rather say that findExe("ninja") should come first. It's the more common naming scheme.

Aleix or Ivan, can you take care of this?

Comment 5 Aleix Pol 2013-12-12 11:05:08 UTC

Git commit 384b2335efc3c69a61914bf3c2874c492e1bbc0f by Aleix Pol, on behalf of Andrew Stitcher.
Committed on 12/12/2013 at 11:02.
Pushed by apol into branch '4.6'.

Find ninja if the binary name is ninja-build

Apparently, some systems name ninja-build to the ninja building tool.
This patch figures out the executable in runtime, so the executable
can be "ninja" or "ninja-build" and still work.

M  +1    -2    projectbuilders/ninjabuilder/kdevninjabuilderplugin.cpp
M  +11   -1    projectbuilders/ninjabuilder/ninjajob.cpp
M  +1    -0    projectbuilders/ninjabuilder/ninjajob.h

http://commits.kde.org/kdevelop/384b2335efc3c69a61914bf3c2874c492e1bbc0f

Comment 6 Aleix Pol 2013-12-12 11:07:08 UTC

Hi Andrew! I just pushed the patch, thank you! :)

PS: I'd suggest that next time you send it to http://reviewboard.kde.org

Comment 7 Andrew Stitcher 2013-12-12 16:49:59 UTC

(In reply to comment #4)
> Patch looks fine, but i'd rather say that findExe("ninja") should come
> first. It's the more common naming scheme.

Kinda irrelevant now that Aleix already pushed the change: Searching for "ninja-build" needs to come before searching for "ninja" because if they are both present then the "ninja" executable is actually a game not the build system!

Comment 8 Kevin Funk 2013-12-12 17:49:15 UTC

Yeah. You're right, it makes more sense to search for the more specific first, for the reasons you mentioned.