Disable plugin project config if project without a IBuildSystemManager dep
ClosedPublic

Authored by kossebau on Nov 15 2018, 9:58 PM.

Details

Summary

The Clazy plugin relies on the project providing a buildsystem manager
to query for the toplevel build directory. If a project has no such manager,
showing the Clazy plugin config pages in the project settings has no
purpose.
As a session might contain different projects, some with a buildsystem
manager, some without, simply disabling the Clazy plugin might not be
wanted for the projects with a buildsystem manager.
So we need to have some condition whether to show some plugins per-project
config pages or not.
For a start, motivated by the crash with the Clazy plugin which has a
hard assumption of project->buildSystemManager() being != nullptr,
this patch adds to the code to collect the config pages for the project
settings dialog some special code which makes use of the plugin metadata
to skip plugins which require a org.kdevelop.IBuildSystemManager, if
there is no buildsystem manager available for the project.
As well adds the (so far missing) entry
X-KDevelop-IRequired: [org.kdevelop.IBuildSystemManager]
to the metadata.

BUG: 400769

Test Plan

Clazy config is no longer shown in the project settings dialog if the
project is used with the generic project manager.

Diff Detail

Repository
R32 KDevelop
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
kossebau created this revision.Nov 15 2018, 9:58 PM
Restricted Application added a project: KDevelop. · View Herald TranscriptNov 15 2018, 9:58 PM
Restricted Application added a subscriber: kdevelop-devel. · View Herald Transcript
kossebau requested review of this revision.Nov 15 2018, 9:58 PM
apol accepted this revision.Nov 15 2018, 11:51 PM
This revision is now accepted and ready to land.Nov 15 2018, 11:51 PM
kfunk added a subscriber: kfunk.Nov 16 2018, 7:03 AM

Nice non-BIC solution, I think!

Thanks for working on this.

This revision was automatically updated to reflect the committed changes.
rjvbb added a subscriber: rjvbb.Nov 17 2018, 10:46 AM

I'm late to this party, but what about projects where the user generates the missing compile_commands.json file manually, e.g. via the compiledb utility? Does this change mean clazy analysis is now possible only for projects where you do NOT need to generate that json file by hand?

I'm late to this party, but what about projects where the user generates the missing compile_commands.json file manually, e.g. via the compiledb utility? Does this change mean clazy analysis is now possible only for projects where you do NOT need to generate that json file by hand?

Not supported, since in your case KDevelop simply does not know a compile_commands.json exist, where it is located to begin with, etc.. It usually only knows that by leveraging IBuildSystemManager infrastructure.

I'm not sure how your (very special) use-case can be addressed. Either way, we'll need to find a way to go through IBuildSystemManager. Maybe we're going to need a build manager plugin which is only fed by a compile_commands.json, nothing else. Would also help realizing idea 2 of https://mail.kde.org/pipermail/kdevelop/2018-October/019723.html.

Not supported, since in your case KDevelop simply does not know a compile_commands.json exist,

Not KDevelop, but last time I looked the clazy plugin accesses and processes the file directly, and it worked just fine with a compile_commands.json file generated with compiledb. So maybe less "not supported" than you thought :)

I'm not sure how your (very special) use-case can be addressed.

I'm not sure if it's so very special at all. There are at least 2 utilities that create a compile_commands database from a Makefile, including the aforementioned compiledb. They do that by wrapping make and analysing the output, so it's trivial to integrate them. Just set the make command to compiledb make, basically.

Would also help realizing idea 2 of https://mail.kde.org/pipermail/kdevelop/2018-October/019723.html.

I also posted about something like this at about the same time ("compiledb-generator and the generic Makefile proj.manager"). Francis Herne replied about a patch he'd written for someone, possibly the author of that 019723.html post.

Not supported, since in your case KDevelop simply does not know a compile_commands.json exist,

Not KDevelop, but last time I looked the clazy plugin accesses and processes the file directly, and it worked just fine with a compile_commands.json file generated with compiledb. So maybe less "not supported" than you thought :)

That must have been magic code then, quick, try to catch it, we all want that ;)

As for the clazy plugin to be able to locate the very compile_commands.json file for accessing, it needs to have a clue where it is located in the filesystem, non? Which usually is the toplevel of the build directory is, and that could be anywhere the user as the user was fancy.

Cmp. the constructor of JobParameters: https://phabricator.kde.org/source/kdevelop/browse/master/plugins/clazy/jobparameters.cpp$144

And yes, for the future we perhaps might want to extend the config UI to allow the user explicitly setting the location of the compilation database, in case there is no support by the buildsystem manager plugin used with the project.
This patch here was just for fixing the referenced crash in 5.3 without changing the UI and doing other bigger principal changes.

rjvbb added a comment.Nov 20 2018, 8:40 AM
That must have been magic code then, quick, try to catch it, we all want that ;)

Absolutely no coding involved, you must have misread me.

As for the clazy plugin to be able to locate the very compile_commands.json file for accessing, it needs to have a clue where it is located in the filesystem, non?

Yes, evidently the plugin expects (expected?) that file to be at the location where the cmake plugin puts it (*), i.e. alongside the CMakeCache.txt file.

*) not during the initial project import in cmake server mode, apparently, though that does create the CMakeCache.txt file. OT here but I've had some other issues with that.

And yes, for the future we perhaps might want to extend the config UI to allow the user explicitly setting the location of the compilation database

Even I see no need for that. Top of the build directory should be fine, so that a full build reset is just deleting that directory.

rjvbb added a comment.Nov 20 2018, 9:34 AM

Found back my ML post entitled

("compiledb-generator and the generic Makefile proj.manager"). Francis Herne replied

*this*:

similar idea was mentioned on IRC a while ago, in that case it was using [Bear]

The user did manage to get a "CMake" project working from their generated
compile_commands.json after applying this [1] hacky patch (which I wrote
the initial version of), and it reportedly worked better than the 'Custom Make'
plugin without needing hand-entered include paths etc.

+1, this seems like a good idea.

[1] https://gist.github.com/Plagman/cf43d9f55099e98184b3e35333768805

@rjvbb : If you generate a compile_commands.json manually or with some tool, the best option for KDevelop (as I mentioned in that email) is to import it as a "CMake" project.

The CMake buildsystem plugin doesn't really care where the JSON file came from (just refrain from using 'configure'), and it handles include paths etc. properly.

Since the CMake plugin does implement IBuildSystemManager, this patch won't disable Clazy in that case.