D7930: Kdevelop CMake plugin : use canonical paths to build.dir

[René J.V. Bertin]

rjvbb added a comment.


  You did see the issue in the past, judging from that referenced commit message.
  
  I'm not changing paths after the fact voluntarily, but you could say that's what I'm doing with creating a shortcut link to the build directory, in a way. The result of that is that certain paths can be changed after the fact by cmake itself (easily visible in the directory entries in compile_commands.json). But it's KDevelop who causes that to happen (or the Makefile), not me directly.
  
  But even if you use the application in a standard workflow where all development, cmake'ing and make'ing takes place from inside the IDE you can still run into this problem. All it takes is that the build system calls a utility (say, moc) which canonicalises its work directory. Those symlinks on the paths used in (C)Makefiles will then all of a sudden behave like they are actual directories, and the 2 paths to the current workdir will not only be different in a string comparison but also in terms of depth w.r.t. the root. It's the latter aspect that breaks header inclusion using relative paths, of course. And that can (will!) happen if you're never using KDevelop at all to work on or build its own sources.
  
  I must admit that I have absolute no idea how to write a unittest for this, in part because it's something I rarely do and above all because I fail to understand why this breaks the KDevelop project and no other that I've identified for now. The initial version of the patch would have made it trivial to test both forms in all existing cmake plugin unit tests, but we'd still need a sensible example that uses a build.dir with a symlink on the path. But ideally you'd need a parser unittest for cmake-based projects, no?
  Right now the only unittest I can think of is setting up a project as described for KDevelop's source, and detecting parsing errors in it or simply building it.
  
  I'm also not sure how this could break in the future. What this patch does is to make sure that cmake (and make) are called *in the actual directory where they would have been called if it weren't for that symlink*. That should never break anything, as long as QFileInfo::canonicalFilePath() isn't broken.
  
  Thinking aloud: this kind of patch isn't required on OS X because there paths are made canonical just about everywhere by the system. I can enter a build.dir of the form `~/work/symlink2work/build` by hand in the cmake project configuration dialog, but not with one of the UI widgets. Similarly, `cd ~/work/symlink2work/build ; pwd` will always report the actual destination directory, and that's also what the builddir combox drop-down will show.
  
  Let's assume that the KDevelop::Path is used systematically throughout the code (can anyone even hope to vouch for that?) AND that in order to use the stored path one always uses a known set of member functions (toLocalFile() being the one I gotten to know). This patch could then be replaced by a global KDevelop::Path option to make the class return canonical paths systematically (maybe that's what Aleix really had in mind?). In turn that should remove the need to write a specific unittest for the cmake although the fact remains a unittest should exist that tests things in presence of a symbolic link.
  
  FWIW, it's also not like I have more time to spend on this than anyone else here. If this patch hadn't addressed the parser issues I was seeing I would probably have gone back to using 5.1.2 until further notice, to get back to the things I had planned for last week...

REPOSITORY
  R32 KDevelop

REVISION DETAIL
  https://phabricator.kde.org/D7930

To: rjvbb, #kdevelop, apol
Cc: mwolff, apol, kdevelop-devel, geetamc, Pilzschaf, akshaydeo, surgenight, arrowdodger
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20170922/83d5235d/attachment.html>