Bug 388370 - Make parser code usable from Umbrello (or other projects)
Summary: Make parser code usable from Umbrello (or other projects)
Status: RESOLVED FIXED
Alias: None
Product: umbrello
Classification: Applications
Component: general (show other bugs)
Version: 2.24.1 (KDE Applications 17.12.1)
Platform: Other Linux
: NOR wishlist
Target Milestone: ---
Assignee: Umbrello Development Group
URL:
Keywords:
Depends on:
Blocks: 373932
  Show dependency treegraph
 
Reported: 2017-12-30 21:33 UTC by Ralf Habacker
Modified: 2018-04-04 09:08 UTC (History)
6 users (show)

See Also:
Latest Commit:
Version Fixed In:
Sentry Crash Report:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Ralf Habacker 2017-12-30 21:33:10 UTC
php import depends on the kdevelop php parser, named kdevelop5-plugin-php5 package on opensuse. Unfortunally the cmake build system of this package does not have support for installing a development package, which are required to build the php import sources.

Another option is to include a copy of the kdev-php kf5 variant sources into umbrello git repo as done for KDE4.
Comment 1 Christophe Marin 2017-12-31 00:08:27 UTC
I'm not sure what "the cmake build system of this package" refers to.
If you're talking about our %cmake macro in the spec file, it just... runs CMake with some default values for a couple vars.
Comment 2 Francis Herne 2017-12-31 00:35:03 UTC
This report is a bit strange, I've tried to make sense of it.

To be clear about the current state:

Umbrello currently vendors in the complete source of a KDE4-based kdev-php version: https://cgit.kde.org/umbrello.git/tree/lib/kdev4-php .

It does so because the "PHP import" feature of Umbrello uses kdev-php's parser and visitor framework (only - not the DUChain, afaict) to extract information from PHP source code: https://cgit.kde.org/umbrello.git/tree/umbrello/codeimport/phpimport.cpp .

Presumably, the intent is that the KF5 port should link against kdev-php's parser without vendoring it as before.

----

Response:

No part of kdev-php is currently intended to be linked against by any external project; hence CMake doesn't install any headers and distros don't ship them in "development packages".

This isn't by a mere oversight; there's no stable API (let alone ABI) and it's not developed with that in mind.

It might be possible to share this code somehow, but you need to discuss with the maintainers about the interfaces needed and how to approach it. Installing an arbitrary set of internal headers won't solve anything.
Comment 3 Sven Brauch 2017-12-31 00:45:40 UTC
I agree with Francis, and I think we should re-assign this to the Umbrello people (doing so now). It's cool that they are able to re-use code from KDevelop, but the packaging is an issue which is in the first place a problem on their side, or at least starts its discussion lifecycle there.
Comment 4 Ralf Habacker 2017-12-31 09:08:33 UTC
Thanks for your response; Francis described exactly the use case. 

I initially added this bug to umbrello as a reminder, but recognized quickly that there is something missing outside umbrello (some kind of development package including headers and cmake find package support). Vendoring kdev4-php solved all required headers for KDE4, which let me think that this bug needs to be assigned to kdev-php, therefore I added the maintainer of kdev-php to this bug for a decision.
Comment 5 Ralf Habacker 2018-01-02 10:32:59 UTC
(In reply to Sven Brauch from comment #3)
> but the packaging is an issue which is in the first place a
> problem on their side, 

I do not agree to this - if umbrello does not embed kdev-php it depends on an external package which need to be imported on compiling by using find_package(kdev-php) or similar.

To be able to discuss some details I collected the required include files 
for https://cgit.kde.org/umbrello.git/tree/umbrello/codeimport/phpimport.cpp.
Beside kdevplatfom includes umbrello requires the following include files from  kdev-php source tree:

parser/parsesession.h"
parser/php.g"
parser/phplexer.h"
parser/tokenstream.h"

From the kdev-php5 build tree it requires the following files
parser/parserexport.h"
parser/phpast-fwd.h"
parser/phpast.h"
parser/phpdebugvisitor.h"
parser/phpdefaultvisitor.h"
parser/phpparser.h"
parser/phptokentext.h"
parser/phptokentype.h"
parser/phpvisitor.h"
Comment 6 Ralf Habacker 2018-01-02 15:04:25 UTC
(In reply to Ralf Habacker from comment #4)
> some kind of ... cmake find package support

This requires to extend function kdevplatform_add_plugin to support "EXPORT KDevPHPTargets", which is located at https://cgit.kde.org/kdevplatform.git/tree/cmake/modules/KDevPlatformMacros.cmake?h=5.1#n131 until version 5.1 andhttps://cgit.kde.org/kdevelop.git/tree/kdevplatform/cmake/modules/KDevPlatformMacros.cmake#n110 for master.

BTW: Just wondering why kdevplatform has been moved inside kdevelop, as it is been used by umbrello since June 2017 (see https://cgit.kde.org/umbrello.git/commit/lib/kdev4-php?h=Applications/17.08&id=b64583e79b554b820e9de8ab0db604c605a76532). Isn't the modularity one of the strength of KDE libraries ?
Comment 7 Sven Brauch 2018-01-02 15:08:28 UTC
Hm, we were not aware of Umbrello using KDevPlatform at the time where we decided to merge the two repos. We were under the impression that nobody was using it at all. Effectively, this means you depend on KDevelop now instead of KDevPlatform, which I can see how it is a bit unfortunate (but not the end of the world).

What you suggest makes sense, but the thing is that this requires the kdev-php maintainer(s) to make some kind of API/ABI guarantee of the exported functions, unless you want it to potentially randomly break every three weeks. They need to be asked whether they can/want to do this ...
Comment 8 Ralf Habacker 2018-01-02 20:07:15 UTC
(In reply to Sven Brauch from comment #7)
> Hm, we were not aware of Umbrello using KDevPlatform at the time where we
> decided to merge the two repos. We were under the impression that nobody was
> using it at all. Effectively, this means you depend on KDevelop now instead
> of KDevPlatform, which I can see how it is a bit unfortunate (but not the
> end of the world).
Then the dependency needs to be 'kdevelop' if kdevplatform > 5.1 is required, because at least in recent opensuse distro there is no kdevelop-devel or kdevelop-kdevplatform-devel package (see https://build.opensuse.org/package/view_file/openSUSE:Leap:42.3/kdevelop5/kdevelop5.spec?expand=1)
It would be easier to have a dedicated kdevelop-kdevplatform-devel, but this seems to be a distribution issue.

> What you suggest makes sense, but the thing is that this requires the
> kdev-php maintainer(s) to make some kind of API/ABI guarantee of the
> exported functions, unless you want it to potentially randomly break every
> three weeks.

I guess this could be solved by installing related files into a version specific private folder similar to what Qt5 does. Those files are only valid for the given version and may be different on other versions.

At a first look the header files from the build dir will fall into this category and should be installed into <include-dir>/kdev-php/private/x.y/ while the files from the source dir could got into <include-dir>/kdev-php ?

cmake build system then needs to add this include path <include-dir>/kdev-php/private/x.y to the generated target interfaces in the cmake find package config files.

> They need to be asked whether they can/want to do this ...

I added the recent kdev-php maintainer to this bug, but did not get any answer. May be I should add Milian too ?
Comment 9 Ralf Habacker 2018-01-02 20:29:09 UTC
(In reply to Ralf Habacker from comment #8)
> Then the dependency needs to be 'kdevelop' if kdevplatform > 5.1 is
> required, because at least in recent opensuse distro there is no
> kdevelop-devel or kdevelop-kdevplatform-devel package (see
> https://build.opensuse.org/package/view_file/openSUSE:Leap:42.3/kdevelop5/
> kdevelop5.spec?expand=1)
> It would be easier to have a dedicated kdevelop-kdevplatform-devel, but this
> seems to be a distribution issue.

Unfortunally this would require to have whole kdevelop including dependencies as build dependency on Windows :-(

This will result - at least on windows - into a dedicated kdevplatform package which uses kdevelop source and builds/installs only the kdevplatform sub dir.
Comment 10 Ralf Habacker 2018-01-02 22:42:04 UTC
A related review request has been opened with https://phabricator.kde.org/D9624. I added Milian Wolff as reviewer because Niko Sams is not available as reviewer. If other or additional reviewer(s) would be better, let me know.
Comment 11 Kevin Funk 2018-03-22 06:29:31 UTC
Git commit 5722388a820f9fcb26c05ade9b1b235a390b0949 by Kevin Funk, on behalf of Ralf Habacker.
Committed on 22/03/2018 at 06:25.
Pushed by kfunk into branch 'master'.

Install parser headers and cmake config files to support client packages

Summary:
Generated headers, which may be required by client packages, are installed
into a private and version specific include dir to indicate that they may
not be stable.

To use the parser in client packages simply add kdevphpparser with
target_link_libraries to a target. Parser related headers need to be
prefixed with <parser/...> e.g. #include <parser/tokenstream.h>.

Test Plan: compiled on linux, verified with patched umbrello build

Reviewers: pprkut!, kfunk, mtijink

Subscribers: brauch, kdevelop-devel

Differential Revision: https://phabricator.kde.org/D9624

M  +39   -9    CMakeLists.txt
A  +4    -0    KDevPHPConfig.cmake.in
M  +1    -1    completion/CMakeLists.txt
M  +1    -1    duchain/CMakeLists.txt
M  +28   -1    parser/CMakeLists.txt
M  +2    -2    parser/php.g

https://commits.kde.org/kdev-php/5722388a820f9fcb26c05ade9b1b235a390b0949
Comment 12 Christophe Marin 2018-03-24 10:07:15 UTC
Note that kdev-php doesn't build anymore (neither locally or in our git-based package in openSUSE:

Scanning dependencies of target kdevphpparser
make[2]: *** No rule to make target 'parser/phpparser.cpp', needed by 'parser/CMakeFiles/kdevphpparser.dir/phpparser.cpp.o'.  Stop.
make[1]: *** [CMakeFiles/Makefile2:312: parser/CMakeFiles/kdevphpparser.dir/all] Error 2