Review Request 122907: Replace "vcsexport.h" with <vcs/vcsexport.h>

[Kevin Funk]

> On March 11, 2015, 7:03 p.m., Andreas Pakulat wrote:
> > No, thats the wrong fix. This requires that when building kdevplatform the top-level source dir is in the include path which is just wrong and it can break in cases where there are multiple vcs directories in the various include paths passed to the compiler via -I (and a vcsexport.h in both). The proper fix is to keep the "" so the header is searched first based on the path of the file including it and using a path that is relative to the file including it. So in vcs/models/foo.cpp use #include "../vcsexport.h" instead of #include "vcsexport.h".
> 
> Giorgos Tsiapaliokas wrote:
>     I understand the problem that this patch creates, but I don't understand how to solve it.
>     
>     > So in vcs/models/foo.cpp use #include "../vcsexport.h" instead of #include "vcsexport.h".
>     
>     Do you mean in vcs/models/foo.h ?
>     
>     I used #include "../vcsexport.h" in vcs/models/foo.[h, cpp] but the build fails.
> 
> Aleix Pol Gonzalez wrote:
>     Maybe it will be just easier if we use the same directory structure both in-source and installed. i.e. installing models into vcs/models/
> 
> Andreas Pakulat wrote:
>     Oh, wasn't aware of that. Yeah, keeping the directory structure is definetly needed for my proposed fix.
>     
>     As for the build issue, is vcsexport.h in the builddir not the source dir? Thats the only way I could see the build to fail. Ah, yeah kf5 generates the header. So using "" with a relative path is not an easy fix either. That means there's no way to fix this properly. As far as I can see at least for the language library a <language/languageexport.h> is used already, so there's a precedent.
>     
>     So I withdraw my objection, just use <vcs/vcsexport.h>. The clash with -I is relatively unlikely anyway since its generally not advisable to have system-provided kdevplatform headers along with a self-compiled version.

+1.

There's no easy way around it (for the exact reasons you mention: a) project layout is different from the install layout, b) headers in the build dir).

So let's just use `<mylib/mylibexport.h>`.


- Kevin


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/122907/#review77314
-----------------------------------------------------------


On March 11, 2015, 4:30 p.m., Giorgos Tsiapaliokas wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/122907/
> -----------------------------------------------------------
> 
> (Updated March 11, 2015, 4:30 p.m.)
> 
> 
> Review request for KDevelop.
> 
> 
> Repository: kdevplatform
> 
> 
> Description
> -------
> 
> Replacing "vcsexport.h" with <vcs/vcsexport.h> allows other projects to use KDev::Vcs
> 
> In another project if your try to use a header which isn't located in the root vcs directory
> like $KF5/include/kdevplatform/vcs the build will fail, 
> 
> because the files located in the subdirectories of $KF5/include/kdevplatform/vcs
> will try to do #include "vcsexport.h" which it doesn't exist. For example
> $KF5/include/kdevplatform/vcs/models/vcseventmodel.h so the build fails.
> 
> Instead <vcs/vcsexport.h> which expands to something like 
> $KF5/include/kdevplatform/vcs/vcsexport.h exists.
> 
> 
> Diffs
> -----
> 
>   vcs/interfaces/icontentawareversioncontrol.h c938468 
>   vcs/interfaces/ipatchdocument.h 998c184 
>   vcs/interfaces/ipatchexporter.h 1a9cce0 
>   vcs/interfaces/ipatchsource.h eaaf5a1 
>   vcs/models/vcsannotationmodel.h b034729 
>   vcs/models/vcseventmodel.h 5ef69f0 
>   vcs/models/vcsfilechangesmodel.h 3d87180 
>   vcs/models/vcsitemeventmodel.h 19ee034 
>   vcs/widgets/standardvcslocationwidget.h 0ad1844 
>   vcs/widgets/vcscommitdialog.h a1cf712 
>   vcs/widgets/vcsdiffpatchsources.h 601e37a 
>   vcs/widgets/vcsdiffwidget.h c2c60fd 
>   vcs/widgets/vcseventwidget.h 336d975 
>   vcs/widgets/vcsimportmetadatawidget.h 4363df9 
>   vcs/widgets/vcslocationwidget.h d1cf35f 
> 
> Diff: https://git.reviewboard.kde.org/r/122907/diff/
> 
> 
> Testing
> -------
> 
> 1. Try to build a project which is using KDev::Vcs, the build will fail
> 2. uninstall kdevplatform and kdevelop
> 3. make clean in kdevplatform and kdevelop
> 4. apply this patch
> 5. build kdevplatform, it builds normally
> 6. build kdevelop, it builds normally
> 7. build the other project, it builds normally
> 
> 
> Thanks,
> 
> Giorgos Tsiapaliokas
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20150312/e8f991d6/attachment.html>