Bug 328695 - [PATCH] Ninja builder is hardwired to use "ninja" executable
Summary: [PATCH] Ninja builder is hardwired to use "ninja" executable
Status: RESOLVED FIXED
Alias: None
Product: kdevelop
Classification: Applications
Component: Build tools: Ninja (show other bugs)
Version: 4.6.0
Platform: Fedora RPMs Linux
: NOR major
Target Milestone: 4.6.1
Assignee: kdevelop-bugs-null
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-12-12 07:12 UTC by Andrew Stitcher
Modified: 2017-02-16 11:07 UTC (History)
2 users (show)

See Also:
Latest Commit:
Version Fixed In:


Attachments
Patch which fixes the problem for me (3.17 KB, patch)
2013-12-12 07:19 UTC, Andrew Stitcher
Details
Somewhat improved patch (3.12 KB, patch)
2013-12-12 07:50 UTC, Andrew Stitcher
Details

Note You need to log in before you can comment on or make changes to this bug.
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.