Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

CxxIssuesReportSensor: use realPath as part of path normalization #1653

Merged
merged 5 commits into from
Jan 6, 2019
Merged

CxxIssuesReportSensor: use realPath as part of path normalization #1653

merged 5 commits into from
Jan 6, 2019

Conversation

ivangalkin
Copy link
Contributor

@ivangalkin ivangalkin commented Jan 3, 2019 •
edited
Loading

  • introduce the fallback for the lookup of InputFiles: if generic lookup was not successful, try it with the Path::realPath()

resolves #1651

@rudolfgrauberger could you please test the snapshot on your local setup?
@guwirth how do we suppose to test things like that?

This change is Reviewable

@ivangalkin ivangalkin mentioned this pull request Jan 3, 2019
Closed
@rudolfgrauberger
Copy link
Contributor

I have tested your changes to some projects and it works perfektly. Works no matter how I write the folder name now e.g. "Sources", "sources" or "SouRcEs". As I expect it under Windows.

Thank you very much!

@guwirth
Copy link
Collaborator

guwirth commented Jan 4, 2019 •
edited
Loading

@ivangalkin I like the solution. Question is if there is a big performance impact? Think not all users need it?

how do we suppose to test things like that?

Think we have two CI systems with Travis and AppVeyor on Linux and Windows. We can test it by adding an integration test.

@ivangalkin
CxxIssuesReportSensor: use realPath as part of path normalization
8d2c035 
* introduce the fallback for the lookup of `InputFile`s:
  try it with the Path::realPath()

resolves SonarOpenCommunity#1651
@ivangalkin
Copy link
Contributor Author

@guwirth I came to the conclusion, that using the real path (Path::toRealPath()) is as error-prone as using canonical path ( #1574/ #1575 ). So I believe it's better not to rely on the real path completely, but rather to use it as a "second try". In that case there is no performance impact for the existing paths. But yes, the query of non-existing path requires more overhead now.

I like the idea to use it in CxxIssuesReportSensor::getInputFileIfInProject(), since the normalization might be useful for more than one sensor. If it's not true, we can tweak VC-related sensor only.

@guwirth guwirth added this to the 1.2.2 milestone Jan 5, 2019
@guwirth
Copy link
Collaborator

guwirth commented Jan 5, 2019

@ivangalkin yes makes sense. Do you like to write a test or should I merge it as it is?

rudolfgrauberger
rudolfgrauberger reviewed Jan 5, 2019
View reviewed changes
cxx-sensors/src/main/java/org/sonar/cxx/sensors/utils/CxxIssuesReportSensor.java Outdated
@@ -145,8 +145,8 @@ private InputFile getInputFileTryRealPath(SensorContext sensorContext, String pa
realPath = absolutePath.toRealPath(LinkOption.NOFOLLOW_LINKS);
} catch (IOException e) {
if (LOG.isDebugEnabled()) {
LOG.debug("Unable to get the real path: module {}, baseDir {}, path {}", sensorContext.module().key(),
sensorContext.fileSystem().baseDir(), path);
LOG.debug("Unable to get the real path: module '{}', baseDir '{}', path '{}', excepiton '{}'",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is an spelling error in 'excepiton'

@ivangalkin
CxxIssuesReportSensor: introduce short-cut in getInputFileTryRealPath()
f2d41b9 
* if the given path cannot be found in module we searched after the "real" path
* shortcut: no need to perform the second search i the "real" path is equal to the given one
* this shortcut should save a bit of run-time for the file-systems, which are
  case-sensitive (e.g. for the mainstream linux)
@ivangalkin
CxxIssuesReportSensor: fix typo
53908c4
@ivangalkin
CxxIssuesReportSensor: fix typo2
1960b41
@ivangalkin
Copy link
Contributor Author

@rudolfgrauberger thx
@guwirth it would be great to merge the PR, since the functionality is hard to unit-test. Also we don't change the primary lookup, but rather add the secondary one. So at least we don't break the main logic.

W.r.t. the performance: I've added a shortcut in order to skip the secondary lookup if it doesn't make sense anymore: f2d41b9. We skip a call into the Plugin-API, so it should safe some run-time. No measurements were made, tho.

@guwirth guwirth merged commit 330a2c3 into SonarOpenCommunity:master Jan 6, 2019
@ivangalkin ivangalkin mentioned this pull request Jan 28, 2019
Closed
@guwirth guwirth mentioned this pull request Feb 8, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rudolfgrauberger rudolfgrauberger rudolfgrauberger left review comments

@guwirth guwirth guwirth approved these changes

Assignees

@ivangalkin ivangalkin

Labels
enhancement
Milestone
1.2.2
Development

Successfully merging this pull request may close these issues.

WARN: Cannot find the file '...' in module '...' base dir '...', skipping violations.
3 participants
@ivangalkin @rudolfgrauberger @guwirth