[kdev-clazy] : use canonical paths
ClosedPublic

Authored by rjvbb on Sep 27 2018, 1:50 PM.

Details

Summary

The clazy plugin currently raises an error (dialog) when it cannot find the subject of an individual file check in compile_commands.json. Some reasons why this can go wrong even if the file in question is still in its original location:

  • the source dir was moved and replaced with a symlink to the new location
  • KDevelop accesses the file via a different path as cmake did when it generated compile_commands.json (on systems where location on disk can have more than a single unique/canonical/normalised $PWD). The user may have opened it via a different access path, for instance, or executed cmake outside of KDevelop.

This patch converts local file paths to their canonical (normalised) version before comparison (and before storing them in the list of project files). As a result, the plugin will be able to check every file that is actually part of the current project.
The conversion is done internally only so only the plugin's behaviour is modified.

Test Plan

Tested with multiple configured projects and the 3 scenarios mentioned in the summary. Individual file analysis fails without the patch but succeeds with.

Diff Detail

Repository
R32 KDevelop
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
rjvbb requested review of this revision.Sep 27 2018, 1:50 PM
rjvbb created this revision.
antonanikin requested changes to this revision.Sep 29 2018, 9:53 AM
antonanikin added a subscriber: antonanikin.

Hi, René. Thanks for patch, but your new version also produce new problems :)

Steps to reproduce:

  1. Open some project, create empty src/test.cpp source file, add it into CMakeLists.txt.
  2. Rebuild and start clazy check for src directory - all works well
  3. Move test.cpp into project's root and create symlink test.cpp -> src/test.cpp
  4. Run clazy again for src directory - error happens.

So if we really want handle all possible symlink-cases (like my test and your variants) we should perform additional actions when directory check selected. We should retrieve (recursive) all directory's child files (with using KDevelop::ProjectModel for example) and use it's canonical paths to find correspondent compile_commands.json elements.

This revision now requires changes to proceed.Sep 29 2018, 9:53 AM

I think there might be a simpler solution to that situation

  • don't normalise paths when creating the list of commands from compile_commands.json
  • do 2 comparisons when checking individual files: 1 of the verbatim paths (i.e. the current check), and another of the canonicalised paths; if either succeeds it's a hit. I could have thought of that myself: my variant of the check might indeed fail in certain cases where the current check succeeds.

(I could say your example is contrived, but so are mine so I guess I won't go there :) )

What do you think, are there other cases where both check variants will fail? The solution above looks like it'll be a lot easier to implement (and maintain) than what you describe.

rjvbb updated this revision to Diff 42550.Sep 29 2018, 2:09 PM

Updated as discussed. This variant is less different from stock code than my previous proposal. It complements the current checks with a comparison using canonicalised paths. As a result it should catch all cases that are not caught by either one of the comparisons when performed in isolation.

rjvbb set the repository for this revision to R32 KDevelop.Sep 29 2018, 2:10 PM
mwolff requested changes to this revision.Jan 15 2019, 1:40 PM
mwolff added a subscriber: mwolff.
mwolff added inline comments.
plugins/clazy/jobparameters.cpp
173

you can remove the previous check, since the second necessarily has to include it

180

we don't do this anywhere else - I'd suggest you always operate on canonical paths

note that you may want to pretty print paths before showing them to the user anyways. there's a function somewhere (Project?) that takes a path and turns it into a relative-to-project-root string ready for consumption by the user

This revision now requires changes to proceed.Jan 15 2019, 1:40 PM
rjvbb marked 2 inline comments as done.Jan 17 2019, 3:09 PM
rjvbb added inline comments.
plugins/clazy/jobparameters.cpp
173

I assume you meant the first component of the || expression?

180

Yes, that would be something like

Core::self()->projectController()->prettyFileName(urlDoc->url(), KDevelop::IProjectController::FormatPlain);

(took me way longer than reasonable to remember where I might find it being used ...)

It seems though that this is already being done (ProblemModel::reset()), did I miss a place where that is not yet so?

rjvbb updated this revision to Diff 49725.Jan 17 2019, 3:09 PM
rjvbb marked 2 inline comments as done.

Updated as requested.

rjvbb set the repository for this revision to R32 KDevelop.Jan 17 2019, 3:10 PM
mwolff accepted this revision.Jan 26 2019, 11:14 PM

lgtm now, note that you now need to push to the new gitlab remote at git@invent.kde.org:kde/kdevelop.git

future review requests should also go through gitlab: https://invent.kde.org/kde/kdevelop

This revision was not accepted when it landed; it landed in state Needs Review.Jan 27 2019, 10:32 AM
This revision was automatically updated to reflect the committed changes.
kossebau added inline comments.
plugins/clazy/jobparameters.cpp
165

Why the QStringLiteral("") and not just a plain QString()?

rjvbb added a comment.Jan 29 2019, 3:56 PM

To put it bluntly: you should have asked that 3 months ago ;)
I really can't remember if I had a specific reason or if this is just the result of a search/replace.

Feel free to push a fix if you feel that's needed (no need I take credit for that).

kossebau added a comment.EditedJan 9 2020, 5:13 AM

Hi @rjvbb

I just came across this logic (for reasons) and wonder if the logic does what you intended with the patch:
given the const auto canonicalPathToCheck = checkPathIsFile ? pathInfo.canonicalFilePath() : QStringLiteral("");, when checkPathIsFile is false, canonicalPathToCheck is an empty string.

Which makes those two other logic using that variable behave bogus:
a) (canonicalPathToCheck == projectCanonicalRootPath) will never be true, as in case of being a path to check the project root path is compared against an empty string and if a file it will be false anyway
b) file.startsWith(canonicalPathToCheck) will always be true, so all files are added to m_sources in case we have a path to check

So my questions:
a) should canonicalPathToCheck not always be pathInfo.canonicalFilePath()?
b) is the check of both file.startsWith(m_checkPath) || file.startsWith(canonicalPathToCheck)the intended check?

I assume 2x yes, but given this patch was written by one person and reviewed by another, I am not sure if my first coffee was rather too weak and my brain is operating bogus still ;)

Edit: actually there is also a follow-up question to b
c) should then file == canonicalPathToCheck not also have a variant checking against m_checkPath?

rjvbb added a comment.Jan 9 2020, 9:38 PM

Difficult to wrap my head around this, so much later!

Resuming your musings, aren't you in fact asking

shouldn't it be simply const auto canonicalPathToCheck = pathInfo.canonicalFilePath();

and my answer to that: "could very well be".

I know that I tested the initial version of the patch, but I'm now beginning to wonder if I really checked the 3 scenarios with the final version. I think I did use the clazy plugin once or twice since, but the truth of the matter is that I hardly find use for it.

I'll try to remember to verify again (as is and with the mod above) but I wouldn't hold my breath until I get around to it :(

Thanks for having had a look again, and confirming that I was not seeing things wrongly half-asleep still :) I am about to touch this code in some days anyway, and would twist things then as needed.

rjvbb added a comment.Jan 13 2020, 7:32 AM

So no urgent need for me to revisit the code?

No further need, thanks. I got the assist in your previous reply already I was looking for :)