improved multi module support

[guwirth]

test 1 for #693

[guwirth]

Failing:
Feature: cpp-multimodule-project
Test multimodule project with reports at root of the project

[guwirth]

@ALL: had a closer look to this yesterday. Currently we are using the root folder as baseDir.(https://github.com/SonarOpenCommunity/sonar-cxx/wiki/Path-and-path-separator-issues)

Import reports in multi module configuration
From version 0.9.3 all reports are looked from the project root, instead of the module base dir. This change makes the plugin behavior consistent between all types of different configurations.

This is more or less a copy from the C# plugin pattern.

Looking to https://github.com/SonarOpenCommunity/sonar-cxx/blob/master/sonar-cxx-plugin/src/main/java/org/sonar/plugins/cxx/utils/CxxReportSensor.java there is different behaviour for analyse and saveViolation:

public void analyse(Project project, SensorContext context) 
...
List<File> reports = getReports(conf,
          reactor.getRoot().getBaseDir().getCanonicalPath(),
          fs.baseDir().getPath(),
...

private void saveViolation(Project project, SensorContext context, String ruleRepoKey,
    String filename, String line, String ruleId, String msg) 
...
String root = reactor.getRoot().getBaseDir().getAbsolutePath();
...

Think we have to unify this behaviour for both cases. I also think the search order is also wrong:

1. should be first fs.baseDir().getPath()
2. then reactor.getRoot().getBaseDir().getCanonicalPath()

What are you thinking?

[jmecosta]

I was already thinking that it must be some thing different between reports
and save because the report contains both search. This is in the PR for the
absolute paths.

I can apply the module first and root second on that PR, maybe it's worth
to apply the file search there also and create its.

It would be great someone could prepare a sample project that uses the
multi modules in different way that we have the ir

On Thu, Dec 3, 2015, 11:37 Günter Wirth [email protected] wrote:

@ALL https://github.com/all: had a closer look to this yesterday.
Currently we are using the root folder as baseDir.(
https://github.com/SonarOpenCommunity/sonar-cxx/wiki/Path-and-path-separator-issues
)

Import reports in multi module configuration
From version 0.9.3 all reports are looked from the project root, instead
of the module base dir. This change makes the plugin behavior consistent
between all types of different configurations.

This is more or less a copy from the C# plugin pattern.

Looking to
https://github.com/SonarOpenCommunity/sonar-cxx/blob/master/sonar-cxx-plugin/src/main/java/org/sonar/plugins/cxx/utils/CxxReportSensor.java
there is different behaviour for analyse and saveViolation:

public void analyse(Project project, SensorContext context) ...List reports = getReports(conf,
reactor.getRoot().getBaseDir().getCanonicalPath(),
fs.baseDir().getPath(),...

private void saveViolation(Project project, SensorContext context, String ruleRepoKey,
String filename, String line, String ruleId, String msg) ...String root = reactor.getRoot().getBaseDir().getAbsolutePath();...

Think we have to unify this behaviour for both cases. I also think the
search order is also wrong:

1. should be first fs.baseDir().getPath()
2. then reactor.getRoot().getBaseDir().getCanonicalPath()

What are you thinking?

—
Reply to this email directly or view it on GitHub
#695 (comment)
.

[guwirth]

Think we have two support the following cases:

• absolute and relative paths for reports in sonar-project.properties
• absolute and relative file items in report files
• in case of relative paths & multi module project:
  • relative to module baseDir
  • relative to root baseDir

==> 8 combinations

[jmecosta]

@guwirth the reactor needs to be used imo, there is no other way of finding what is the project root without using it. so the case where we look for reports from the root needs to be supported since it was the only supported case so far and the most users that use multi modules are using this approach,

to be is loads for setup if you need to generated reports for each modules, its much easier to generate reports for all the project and then just put them in a single location in root of the project

[guwirth]

the reactor needs to be used imo, there is no other way of finding what is the project root without using it.

@jmecosta In case we are supporting absolute paths for defining report file locations what's the difference?

# baseDir = x:/root
sonar.cxx.XXX.reportPath=x:/root/reports/xxx-*.xml
sonar.cxx.XXX.reportPath=x:/outside/reports/xxx-*.xml

[jmecosta]

@guwirth usually its not a good idea to hard code absolute paths in props files, it will only work in the machine where it was created. for example our ci system uses Base Paths that change according to git revision... so hard coding those in props will not work for us. we would need to pass those via arguments so that they overwrite what exist in props file

i mean absolute paths are ok, but it must be a machine passing those to the analysis via -D parameters. imo we are supporting this case to enable such flexibility and not to make it a default padron that everyone uses

[guwirth]

@jmecosta with the flat modell the paths in the report files has to be absolute. Means reports working only if they fit to a specific machine. So have one more absolute path doesn't matter?

[jmecosta]

We use relative paths for reports in flat mode, no problems. Perhaps we
talk about differwnt things?

On Sun, Dec 6, 2015, 13:51 Günter Wirth [email protected] wrote:

@jmecosta https://github.com/jmecosta with the flat modell the paths
in the report files has to be absolute. Means reports working only if they
fit to a specific machine. So have one more absolute path doesn't matter?

—
Reply to this email directly or view it on GitHub
#695 (comment)
.

[guwirth]

We use relative paths for reports in flat mode, no problems

On #697 you said: flat (relative to root + absolute)

In case flat is relative then there is still the question in case of multi module:

#697 ... reactor... In case of using always root baseDir: If there are realtive paths in report files are they relative to root baseDir or current baseDir?

[jmecosta]

I'm refering to the patterns we use to search reports, not the content of
the report it self. The patterns are hard coded in property file and making
it absolute will not work in ci system.

The path inside report indeed have to be absolute, but those are generated
during analysis and we have our tools setup to report only in absolute paths

On Sun, Dec 6, 2015, 13:52 Jorge Costa [email protected] wrote:

We use relative paths for reports in flat mode, no problems. Perhaps we
talk about differwnt things?

On Sun, Dec 6, 2015, 13:51 Günter Wirth [email protected] wrote:

@jmecosta https://github.com/jmecosta with the flat modell the paths
in the report files has to be absolute. Means reports working only if they
fit to a specific machine. So have one more absolute path doesn't matter?

—
Reply to this email directly or view it on GitHub
#695 (comment)
.

[jmecosta]

To be honest I would not support cases where reports contain only relative
paths there are to many ambiguities. It's easy to tell a static analyser to
produce issues with absolute paths . we should however support absolute
paths for report search path

On Sun, Dec 6, 2015, 14:00 Jorge Costa [email protected] wrote:

I'm refering to the patterns we use to search reports, not the content of
the report it self. The patterns are hard coded in property file and making
it absolute will not work in ci system.

The path inside report indeed have to be absolute, but those are generated
during analysis and we have our tools setup to report only in absolute paths

On Sun, Dec 6, 2015, 13:52 Jorge Costa [email protected] wrote:

We use relative paths for reports in flat mode, no problems. Perhaps we
talk about differwnt things?

On Sun, Dec 6, 2015, 13:51 Günter Wirth [email protected] wrote:

@jmecosta https://github.com/jmecosta with the flat modell the
paths in the report files has to be absolute. Means reports working only if
they fit to a specific machine. So have one more absolute path doesn't
matter?

—
Reply to this email directly or view it on GitHub
#695 (comment)
.

[Bertk]

I agree the absolute paths shall be supported for reports but we should also check the performance and maybe introduce a file/path interface for the plug-in. I found this comment and I have low confidence that this will be improved:

FilePredicate org.sonar.api.batch.fs.FilePredicates.hasAbsolutePath(String s)

    Predicate that gets a file by its absolute path. The parameter accepts forward/back slashes as separator and non-normalized values (/path/to/../foo.txt is same as /path/foo.txt). 

Warning - not efficient because absolute path is not indexed yet.

We should also consider to support a simple, comma separate list of paths for a report configuration item. The ant style search pattern is not a common way.
By the way, I did a small update to support multiple test reports with absolute paths for a component (see 24ed1e2).

[jmecosta]

so we agree on the need for absolute paths for reports, #683 should handle it. 24ed1e2 looks to me that it does not support /abc/def/*.xml however the "," can be added later once the main lookup is clarified and merged to master.

now for allowing relative paths inside reports to raise issues, the only hard rule that we can apply is disallow it (the performance i think it should not be a big issue because we have been using this so far no one as complained?). if we really want to support it then imo would be, relative to project root and if not found relative to module however this would not use the same algorithim as for looking reports but use only the functions provided by the plaform (so that it does not make it slower)

another thing regarding the relative paths is that, we dont know what users are using as base dir to generate reports. file x can be relative to anything, even to a folder outside project. soon they will be asking why my relative path does not work if we support it.

[jmecosta]

i am ensure about this, with this change we are telling people that they must to have their reports always per module and not once at the root of the repo as it has been supported until now. as far as i know this work is mostly manual and you need to generate reports per module instead of generating the reports once for the all repo.

although this might be ok for some, i think we should also support the lazy people that just put reports on root of the repository and let the plugin deal with it. as far as i know the only way of doing it is with the reactor, so i think we should not remove it

[guwirth]

@jmecosta this PR is only to play around and see what is needed to support the SQ default behavior for multi module.

What I like on this solution is that the code is very simple, SQ is supported as described on there page and also absolute and relative path support is included in this solution. What is not working is "our" reactor mode having the reports all relativ to the root folder. This could be solved by setting absolute paths in the root BaseDir config (I know you don't like it because path could be different on different servers). Here I would like to come back to a suggestion I made already in the past: why not supporting environment variables in the config file? (I know that there are plans to support this in sonar runner 2.5 anyway).

What I don't like on your approach #683 currently: it sounds to much like a msbuild runner solution only. I think there are still many other users like to use this plugin without VS and without msbuild runner. We should not reduce this plugin to a msbuild runner extension. (What I like on your approach: code and structure is better as existing one.)

Anyway: as long as your solution is still supporting the "old" use cases we should merge it as soon as the existing unit and integration tests are green (before this one). We can improve/generalize things always later one.

I will also try to add some multi module IT tests in a separate PR to see if default SQ modes are supported.

[jmecosta]

the reactor is only used for looking up reports from root of the project, its of no use to the msbuild runner.

For the msbuild runner the only thing required is to support absolute paths (and those need to be hacked). the msbuild changes this ideas quite a lot, the reports are looked up by default like this.
.sonarqube\out\a1a__Win32_Debug_635855202494571037\analysisresults.xml

12:31:04.669 INFO - Base dir: D:\Development\SonarQube\cxx\sonar-cxx\integration-tests\testdata\cpp-multimodule-project-2
12:31:04.669 INFO - Working dir: D:\Development\SonarQube\cxx\sonar-cxx\integration-tests\testdata\cpp-multimodule-project-2.sonarqube\out.sonar

At least when i tried the first time, all reports that were under
\cxxresults*.xml were picked up from the .sonarqube folder above. this is the reason i started to implement the support of absolute paths.

... just to be sure i really dont use the reactor myself, but i assume that some users do. because its kind of easy to keep those there. (an example, cppcheck works better if you run on root of the directory)

but anywhy i like your suggestion to support this special variable, on the file. but this definetly can be done once the absolute report path is done.

i will get #683 ready, requires unit tests to be re-factored. and then we can review it and if its ok to merge then we can work on the rector as u suggested

[guwirth]

i will get #683 ready, requires unit tests to be re-factored. and then we can review it and if its ok to merge

👍

[guwirth]

This PR is now ready to merge. The reactor is removed but with a modified configuration file the 'reactor multi project' is still working.

This solution is now much closer to the SQ default behavior and there is no more 'magic' functionality inside the plugin. The user has to define projectBaseDir explicit - in reactor use case absolute.

One thing is a little bit disappointing: It's not possible to set a placeholder for projectBaseDir. The sonar runner is checking the value in the EmbeddedRunner long before the plugin can see it (see https://github.com/SonarSource/sonar-runner/blob/master/sonar-runner-api/src/main/java/org/sonar/runner/api/Dirs.java). Don't know if this is possible to modify? Will create a issue in the SQ newsgroup.

@Bertk you are using this PR already?
@jmecosta ok?

[jmecosta]

Ok

On Fri, Jan 1, 2016, 16:36 Günter Wirth [email protected] wrote:

This PR is now ready to merge. The reactor is removed but with a modified
configuration file the 'reactor multi project' is still working.

This solution is now much closer to the SQ default behavior
http://docs.sonarqube.org/display/SONAR/Analyzing+with+SonarQube+Scanner#AnalyzingwithSonarQubeScanner-Multi-moduleProject
and there is no more 'magic' functionality inside the plugin. The user has
to define projectBaseDir explicit - in reactor use case absolute.

One thing is a little bit disappointing: It's not possible to set a
placeholder for projectBaseDir. The sonar runner is checking the value in
the EmbeddedRunner
https://github.com/SonarSource/sonar-runner/blob/master/sonar-runner-api/src/main/java/org/sonar/runner/api/EmbeddedRunner.java
long before the plugin can see it (see
https://github.com/SonarSource/sonar-runner/blob/master/sonar-runner-api/src/main/java/org/sonar/runner/api/Dirs.java).
Don't know if this is possible to modify? Will create a issue in the SQ
newsgroup.

@Bertk https://github.com/Bertk you are using this PR already?
@jmecosta https://github.com/jmecosta ok?

—
Reply to this email directly or view it on GitHub
#695 (comment)
.