[cmake-developers] Correct way to get include directories for a target (automoc regression) ?

[Stephen Kelly]

Alexander Neundorf wrote:

>> > Does moc know actually know about system include dirs ?
>> 
>> I don't know.
> 
> Interestingly, moc seems to fail on some systems if all system include
> dirs are given to it, see the Raphaels comment (
> http://public.kitware.com/Bug/view.php?id=13667#c31553)

Yes, a separate bug in moc was uncovered by this episode:

 https://bugreports.qt-project.org/browse/QTBUG-28045

The KDE builds tester fails similarly:

 http://build.kde.org/job/phonon_master/23/console

and has done since your branch was merged to next (that's the cmake used by 
build.kde.org, which is good to know):

 http://build.kde.org/job/phonon_master/

I experimented with moc a bit and read the code, and it doesn't seem to make 
any effort to ask the compiler what the compiler-implicit include 
directories are. This seems to be intentional. Any includes which can not be 
found are ignored by moc.

> 
>> The reason /usr/lib/qt/include is inserted by phonon into the implicit
>> include directories is that querying g++ returns that path. Is it normal
>> for g++ to return that (Qt specific) path as a system include, or is that
>> resulting from a patch the distros make to g++ based on where they
>> install Qt?
> 
> At least it's also the case on my Slackware 13.37, and I think I didn't
> manually change this.
> ...this could have the interesting effect that I also get
> /usr/lib/qt/include when using some other Qt installation :-/

That's something for you to look into. It seems that several distros are 
doing this, and so far I don't know why. It seems counter-productive to 
deliberately put the headers into a directory which the compiler isn't going 
to search for implicitly, and then add it to the directories that the 
compiler searches implicitly. I'm sure there is some reason for it, but I 
think we need to know what that is.

My Ubuntu system does not add the Qt include dir to g++ (that is my 
conclusion after seeing the output of 'g++ -v -E -x c++ -dD dummy').


>> This to me looks like it's not appropriate for KDE/phonon to populate the
>> implicit include directories. Whatever patch goes into CMake (to handle
>> already-released versions of phonon and others that have copied from
>> FindKDE4Internal), that should probably not be done anymore by
>> kdelibs/phonon.
>> 
>> Removing that macro from phonon would fix the reported issues too, I
>> think, without requiring the users of (trunk versions of) phonon to wait
>> for CMake 2.8.11.
>> 
>> Can your patch be wrapped in some condition to check if
>> FindPhononInternal is being used? I've seen something similar to check
>> VTK versions, so it's not un-heard-of.
> 
> I have put a FixAutomocRegression2 branch now on stage, which is less
> intrusive than the first fix.
> It now checks whether ${QT_QTCORE_INCLUDE_DIR} is being given to moc as an
> include dir, and if this is the case, but ${QT_INCLUDE_DIR} is defined and
> not given to moc, ${QT_INCLUDE_DIR} is added.
> I think for Qt4 this should be safe.

The original problem I had with the patch is that it reduces flexibility for 
the future, and it's not appropriate to that workaround in CMake anyway - I 
think it's more appropriate to remove the manipulation of the 
IMPLICIT_INCLUDE_DIRS as Phonon is doing. That is not an appropriate thing 
to be doing as far as I can see. 

I think there needs to be some consistency in answering the question 'To 
what extent can people using CMake use internals and expect something that 
works once to work always, no matter what, without any maintenance?'. I 
don't think that adding somewhat complex patches to CMake to compensate for 
people using internals is a good idea. If you are using internals, you need 
to expect to do your own maintenance. That was the answer for people 
extending the language support, so I think it should be the answer here too.

That's my view, and I know it is not universally agreed here, but I wanted 
to state it.

Also, the patch may be getting less intrusive in the new topic, but it is 
more complex. I think a full understanding of the origins of the problem is 
needed before getting patches into CMake for 'fixing downstream bugs'.

> The QtCore include dir should always be used when using anything from Qt4,
> right ?

I don't think so.

 find_package(Qt4)
 # Don't include_directories
 add_executable(main main.cpp)
 target_link_libraries(Qt4::Core)

would work if Qt was in /usr/ or another standard prefix, and main.cpp did 
this:

 #include <QtCore/QObject>

Not using include_directories is arguably a mistake though.

(Actually maybe this is why distros add ${QT_INCLUDE_DIR} to the compiler 
implicit include dirs - so they can use code that does that without a 
buildsystem)

> It should also always be a subdir of ${QT_INCLUDE_DIR} and so usually not
> be part of the system-default include dirs.

Until we know why distros add ${QT_INCLUDE_DIR}, we won't know if they will 
add ${QT_INCLUDE_DIR}/QtCore. That would also be 'convenient' if convenience 
is the only reason for adding ${QT_INCLUDE_DIR}.

> I don't know about Qt5, but at least for Qt5 nothing can't be broken
> (since it is not yet released), right ?

Well, with Qt 5 the issue will be hit again if KDE and Phonon continue to 
populate the IMPLICIT_INCLUDE_DIRS and if distros add the Qt 5 include path 
to the compiler (eventually - presumably they can't add both Qt 4 and Qt 5 
include directories at once). 

That's why I keep calling for removal of that use of internals by KDE and 
Phonon, independent of what patches go into CMake. Apparently it is not such 
a useful 'feature' anyway because so much depends on what order things 
appear in include_directories calls in the CMakeLists.txt file, as Raphael 
said:

 http://thread.gmane.org/gmane.comp.kde.devel.buildsystem/5581/focus=7096

People might end up moc'ing with includes from one location and compiling 
with includes from another location.

That even looks like a more-generic problem than just a moc problem. It may 
be something there should be a generic solution for in CMake (or not we'd 
have to explore the actual problem). 

Thanks,

Steve.