Meson plugin: First working version
Needs ReviewPublic

Authored by dmensinger on Sat, Dec 1, 11:09 AM.

Details

Reviewers
apol
Summary

This patch implements the basic functionality of the meson plugin.
Introspection (targets, include dirs, etc.) is not implemented yet.

The current features are:

  • (re)configuring a meson project
  • building with ninja
  • GUI for configuring a build directory (currently only install prefix and build type)
  • managing multiple build directories
  • GUI for adding a new build directory

Diff Detail

Repository
R32 KDevelop
Branch
meson_initial_phabricatorSucks (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 5630
Build 5648: arc lint + arc unit
dmensinger created this revision.Sat, Dec 1, 11:09 AM
Restricted Application added a project: KDevelop. · View Herald TranscriptSat, Dec 1, 11:09 AM
dmensinger requested review of this revision.Sat, Dec 1, 11:09 AM
dmensinger updated this revision to Diff 46611.Sat, Dec 1, 12:33 PM
  • Fixed typo in icons/CMakeLists.txt
Restricted Application added a project: Documentation. · View Herald TranscriptSat, Dec 1, 12:33 PM
Restricted Application added a subscriber: kde-doc-english. · View Herald Transcript
yurchor added inline comments.
plugins/meson/kdevmesonmanager.json
6

There is no need to add translations. They will be removed by scripty then renewed after the translator review anyway.

plugins/meson/mesonbuilder.cpp
151

Typo: seam -> seem

156

Typo: can not -> cannot

plugins/meson/settings/mesonconfigpage.ui
123

Typo: seperate -> separate

plugins/meson/settings/mesonnewbuilddir.cpp
170
188

Typo: can not -> cannot

dmensinger updated this revision to Diff 46614.Sat, Dec 1, 1:27 PM
  • Fixed typos and removed translations
rizzitello added inline comments.
plugins/meson/mesonbuilder.cpp
56

Would ending the string with a \n (or two) remove the need to append an empty QStringLiteral in the next line?

58

QChar::fromLatin1('\n') instead of the older cast ?

185

Else is not needed since the if ends with a return.

plugins/meson/mesonconfig.cpp
185

return !( buildDir.isEmpty() || mesonExecutable.isEmpty() || buildType.isEmpty() ); ?

plugins/meson/mesonimportjob.cpp
48

"sourcePath" and "bulilddir" appear to be unused here.

plugins/meson/settings/mesonconfigpage.cpp
179

auto

dmensinger marked 6 inline comments as done.Sat, Dec 1, 3:11 PM
dmensinger added inline comments.
plugins/meson/mesonimportjob.cpp
48

Yeah, I haven't touched that code yet. This is still from the barebones framework that was created over a year ago. I will replace this function with a stub for the time being.

plugins/meson/settings/mesonconfigpage.cpp
179

I don't know why you would use auto here. Should I use auto for every dynamic_cast?

dmensinger updated this revision to Diff 46623.Sat, Dec 1, 3:17 PM
  • Some code cleanup
rizzitello added inline comments.Sat, Dec 1, 5:50 PM
plugins/meson/settings/mesonconfigpage.cpp
179

In these cases the type is obvious. Why not use auto?

dmensinger marked 4 inline comments as done.Sat, Dec 1, 6:09 PM
dmensinger added inline comments.
plugins/meson/settings/mesonconfigpage.cpp
179

Then I guess it is personal preference :)
I like it better this way for (relatively) short type names without templates and :: (outside C++11 for loops).
If you are OK with it, I would like to keep the types the way they are now.

rizzitello added inline comments.Sat, Dec 1, 6:26 PM
plugins/meson/settings/mesonconfigpage.cpp
179

It's okay with me if you keep the types how they are now.

flherne added inline comments.
plugins/meson/mesonbuilder.cpp
47

This needs setStandardToolView(KDevelop::IOutputView::BuildView); or similar, otherwise the output appears as a separate view with no name or icon.

151

This condition is reached in case a previous configure attempt failed, e.g. because of a missing dependency.

KDevelop refuses to re-try the configure step (after installing the dependency) until the build dir is removed, which is quite inconvenient.

It's quite easy to accidentally start repeated configure jobs, in which case the second one fails - e.g. by clicking "Apply" and then "Ok" in the project settings.

Perhaps if a configure job is already running, it should be cancelled before starting a new one (or the new one ignored if the settings are unchanged?).

Will try to review the actual code sometime.

dmensinger updated this revision to Diff 46704.Sun, Dec 2, 2:00 PM
  • Only configure when config changed
  • Better status handling in the settings UI