Fix #387391: Random responses from the C/C++ language support
Needs RevisionPublic

Authored by vbspam on Dec 12 2017, 2:04 PM.

Details

Reviewers
mwolff
Group Reviewers
KDevelop
Summary

Fix #387391: Random responses from the C/C++ language support when using
symbolic links to CMake sub-projects (https://bugs.kde.org/show_bug.cgi?id=387391)

Although the original idea of using canonicalFilePath() sounds as best solution, it looks like KDevelop uses absoluteFilePath() in C/C++ language support query url [1] to related build system artifact.

[1] file: plugins/clang/clangparsejob.cpp:119 method: ProjectFileItem* findProjectFileItem(const IndexedString& url, bool*hasBuildSystemInfo)
( )

Test Plan

Being new to KDevelop I have no idea what should be here.

Within the bug report I prepared steps to reproduce the bug.

I also prepared minimal and complex projects which may serve as this test plan test cases.

For details please see the bug #387391 (https://bugs.kde.org/show_bug.cgi?id=387391).

I also have a question which I put into the bug report which is related to this patch. Simply: although I know why my solution works, it is not clear why the original solution does not work now. I guess that the query uses newly url with absoluteFilePath instead.

Diff Detail

Repository
R32 KDevelop
Lint
Lint Skipped
Unit
Unit Tests Skipped
vbspam created this revision.Dec 12 2017, 2:04 PM
Restricted Application added a subscriber: kdevelop-devel. · View Herald TranscriptDec 12 2017, 2:04 PM
vbspam requested review of this revision.Dec 12 2017, 2:04 PM
brauch added a subscriber: brauch.Dec 12 2017, 2:53 PM

Hmm. There's a comment in the line above which states it explicitly uses canonicalFilePath to avoid issues with symlinks. If we resolve symlinks like you suggest, there will be situations like files which are part of a project but for which the project's root directory is not a prefix of the file path. Are we somewhat sure this doesn't break in other places?

Hmm. There's a comment in the line above which states it explicitly uses canonicalFilePath to avoid issues with symlinks.

I 100% agree, that is why I am cautious about this, therefore my question above.

If we resolve symlinks like you suggest, there will be situations like files which are part of a project but for which the project's root directory is not a prefix of the file path.

My test case scenario (https://bugs.kde.org/attachment.cgi?id=109155) is covering this. I have one CMake project inside, I have other (LibBar) which is "physically" outside and symlinked inside.

This test scenario works well with the fix now. What I found is that canonicalFilePath() (in other words unique path pointing to the original/physical file - outside the project root), does not work well with KDevelop call which uses absoluteFilePath() (in other words absolute path visually appearing as absolute path to the symlink (not to the target file) - so to be inside project).

Are we somewhat sure this doesn't break in other places?

It works for normal files, it also works for both absolute(don't get confused with absoluteFilePath above which which is this term not about) and relative symlinks.

Thank you for the question! You are right, I forgot to test symlinks to resources physically stored inside the project root. I quickly created such scenario now which can be found here (https://bugs.kde.org/attachment.cgi?id=109341). I reveals that it does not work, I believe however that KDevelop does not properly build the project tree. My new test scenario links "LibAnotherBar" to the project root from the "InsideProjectRoot/LibAnotherBar". The build system is instructed to use the LibAnotherBar (by cmake add_subdirectory(LibAnotherBar) ), KDevelop however assumes the build system meant InsideProjectRoot/LibAnotherBar" and shows the project treeview populated with the "InsideProjectRoot/LibAnotherBar" instead requested "LibAnotherBar".

I did check the CMake server response and it shows properly generated build system (please check attached test4.json

)

vbspam added a comment.EditedDec 12 2017, 6:03 PM

I reveals that it does not work, I believe however that KDevelop does not properly build the project tree.

I forgot to mention that it also does not work with the official mainline KDevelop 5.2.1 from Neon. At this moment I see this as new bug report candidate (UPDATE: I already tested that behavior and filled a bugreport here https://bugs.kde.org/show_bug.cgi?id=387866) I need to test it more. So we may assume that the fix we discuss does not make this test case result worse.

It would be perfect if someone else will try my test cases. Even I do my best to make may environment as clean as possible, I do not have separate system to test it. I tried LXC containers which I usually use for console apps, but exported X display to/from LXC make things slow.

mwolff added a subscriber: mwolff.Jan 5 2018, 12:57 PM

can you turn the test projects into proper unit tests that can be run in an automated fashion to see what happens here?

also the absoluteFilePath call in kdev-clang is only used for the (undocumented) .kdev_pch_include feature - so that is unrelated. It should indeed probably use the canonical file path though. Anyhow, since this is clearly unrelated to what you are trying to fix, I wonder if your change actually helps? Is it maybe the call to absoluteFilePath in plugins/custom-definesandincludes/noprojectincludesanddefines/noprojectincludesanddefinesmanager.cpp's NoProjectIncludePathsManager::findConfigurationFile that should be turned into a canonical path?

In general, it might be a good idea to go through our code base and replace all calls of absoluteFilePath with their canonical equivalents... But let's not do that blindly, rather on a case-by-case study.

mwolff requested changes to this revision.Jan 5 2018, 12:57 PM

needs changes afaik

This revision now requires changes to proceed.Jan 5 2018, 12:57 PM
vbspam added a comment.Jan 6 2018, 8:40 AM

Thank you for your attention.

can you turn the test projects into proper unit tests that can be run in an automated fashion to see what happens here?

I need first learn how to do it, I will let you know when I have something.

You are right, in the meantime I did find that the fix is not solving all my complex test cases. I did some investigation and it looks that sometimes the query for project item resources (https://github.com/vbspam/kdevelop/blob/master/plugins/clang/clangparsejob.cpp#L169) uses absolute and sometimes canonical path representation.

Subjective opinion: I also realized that I do not like the automatic resolving of symbolic links to be presented to the end user as KTextEditor or KATE components are doing (with some example here: https://bugs.kde.org/show_bug.cgi?id=387866#c5 which has I believe consequences described in the bug report). This is not suggestion for change, it is just other view. I can understand that KDevelop may have reasons why is doing it this way. I did realized that when I used QtCreator which does not resolve symlinks and I felt this as more comfortable when thinking about the code (no need to draw attention to locations outside the project).

I will go through by you suggested spots in the code to better understand the issue and maybe try to fix it properly.

I am not sure how "plugins/custom-definesandincludes/noprojectincludesanddefines/noprojectincludesanddefinesmanager.cpp" relates to CMake includes and defines?

I mean, even the fix does not solve the issue, custom defines and includes does not looks like a place to fix it.

When I debugged the issue it always puts me to the "plugins/clang/clangparsejob.cpp:169" where it just do not find the suitable build system info.

I traced it into the CMake "plugins/cmake/cmakemanager.cpp:240" to the call CMakeManager::fileInformation(..) where it sometimes use absolute and sometimes canonical file path. When the collection contains absolute paths, it still can be fixed by manual searching through the "data.files" QHash collection and testing both absolute and canonical file paths. But when the (data.files) collection key contains canonical file paths, it can't be translated to absolute file path of the queried url (url of the original query) - the absolute file path of canonicalized file path is always the same (does not trace back to all possible symbolic links which is pointing to that "real" file).

victorl added a subscriber: victorl.EditedJul 24 2018, 1:06 PM

Just trying to add more information, the background parser fails to parse my ROS projects if the ROS packages (a directory containing a CMakeLists.txt file) are symlinks

I'm using KDevelop 5.2.1 on Kubuntu 18.04

Steps to reproduce

git clone https://gitlab.com/InstitutMaupertuis/topics_rviz_plugin.git $HOME/topics_rviz_plugin
  • Create a catkin workspace with the ROS package as a symlink:
mkdir -p $HOME/catkin_workspace/src
cd $HOME/catkin_workspace/src
catkin_init_workspace
ln -s ../../topics_rviz_plugin

  • Let the background parser finish (shouldn't take more than a few seconds)
  • Open topics_rviz_plugin/src/topic_info.cpp for example, it should be correctly coloured, modify the file (add a whitespace for example) and save, the colours goes away and KDE fails to resolve every ROS include.

Before editing the file

After editing the file

If instead of having a symlink in the catkin workspace ($HOME/catkin_workspace/src/topics_rviz_plugin) you put the directory and import the project again, everything works perfectly.
My conclusion is that the symlink causes the problems but I can be wrong.

Hi Victorl

The fix I proposed do not actually fix the issue.

Also there is not much attention in this issue.

If I correctly understand, your comment is more related to the bug and
not to this obsolete fix.

Would it be possible to put your observation to the bug
https://bugs.kde.org/show_bug.cgi?id=387391 which is tracking all
related information.

Best regards

Venca