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.

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

Enhancement/multi module with reactor #358

Merged

Conversation

jmecosta
Copy link
Member

@jmecosta jmecosta commented Jan 4, 2015

kind of rebase... clean reactor

@wenns
Copy link
Contributor

wenns commented Jan 6, 2015

Now I have played a little bit this this stuff I know better whats going on.

This multi-module thing is a mine field. This PR hacks the plugin to make the modules pull reports which actually meant to be project-level. This has all sorts of effects, e.g.:

  • the xunit simple mode is not working
  • reports are processed N times (1 would be sufficient)
  • reports are producing lots of wanings of type
    'could not find file ' because this file is in an other module
  • project-level violations will be assigned to each module
    etc.

I have mixed feeling with this. Can we do better? Comments?
Meanwhile I'll test the scenario where reports are per module and see how it works.

@jmecosta
Copy link
Member Author

jmecosta commented Jan 6, 2015

the problems you pointed out have been already also pointed out for c# plugin. and in fact they have a ticket http://jira.codehaus.org/browse/SONARVS-59 to make it optional. this would mean that we actually do not need any of this (?). also having mix feelings

am not sure what people that use multimodule not in this way say about. any way...

imo the issues you mention are not a implementation issue of the cxx plugin, rather a way the platform deals with this. any way, you should probably state that when using multi modules all these limitations apply. at least until we figure out

@jmecosta
Copy link
Member Author

jmecosta commented Jan 6, 2015

not sure if this is a good idea, but it prevents issues with duplicated metrics

@jmecosta
Copy link
Member Author

jmecosta commented Jan 6, 2015

yeah this is a mess :) but the alternative for us is to run the analysis per module and save xmls per module. this is a much harder setup.

ive made so now that the xunit sensor in simple mode should run only at root of the project.. added the correspondent bdd.

@jmecosta jmecosta mentioned this pull request Jan 6, 2015
@wenns
Copy link
Contributor

wenns commented Jan 6, 2015

Mess it is... Raw and hacky. And Im not even half through the most important scenarios for multi-module. Also, there is no clear policy or spec from the platform or other plugins, at least none Im aware of. This feature seems to come from the witsh to support maven modules, without much design work.
Anyway. I want to experement further and Im willing to finish it. We'll see.

@jmecosta
Copy link
Member Author

jmecosta commented Jan 6, 2015

not sure whats going on, on window the tests are passing just fine. in unix they are not... apparenty the search function is not able to find the resources

@jmecosta
Copy link
Member Author

jmecosta commented Jan 6, 2015

should now be green... it was a issue with the path separator in the reports

@wenns
Copy link
Contributor

wenns commented Jan 10, 2015

Sorry, will skip this in 0.9.2

@wenns wenns modified the milestones: M 0.9.3, M 0.9.2 Jan 10, 2015
@jmecosta jmecosta modified the milestones: M 0.9.2, M 0.9.3 Jan 10, 2015
@guwirth
Copy link
Collaborator

guwirth commented Apr 12, 2015

This was a lot of work and is an useful feature.
@Bertk If you could do final testing I would merge it after rebase.

@guwirth guwirth added this to the M 0.9.3 milestone Apr 12, 2015
@Bertk
Copy link
Contributor

Bertk commented Apr 12, 2015

I can test it next Sunday only.

Jorge Costa added 2 commits April 12, 2015 21:44
…-module-with-reactor

Conflicts:
	sonar-cxx-plugin/src/main/java/org/sonar/plugins/cxx/compiler/CxxCompilerSensor.java
	sonar-cxx-plugin/src/main/java/org/sonar/plugins/cxx/coverage/CxxCoverageSensor.java
	sonar-cxx-plugin/src/main/java/org/sonar/plugins/cxx/cppcheck/CxxCppCheckSensor.java
	sonar-cxx-plugin/src/main/java/org/sonar/plugins/cxx/externalrules/CxxExternalRulesSensor.java
	sonar-cxx-plugin/src/main/java/org/sonar/plugins/cxx/pclint/CxxPCLintSensor.java
	sonar-cxx-plugin/src/main/java/org/sonar/plugins/cxx/rats/CxxRatsSensor.java
	sonar-cxx-plugin/src/main/java/org/sonar/plugins/cxx/utils/CxxReportSensor.java
	sonar-cxx-plugin/src/main/java/org/sonar/plugins/cxx/valgrind/CxxValgrindSensor.java
	sonar-cxx-plugin/src/main/java/org/sonar/plugins/cxx/veraxx/CxxVeraxxSensor.java
	sonar-cxx-plugin/src/main/java/org/sonar/plugins/cxx/xunit/CxxXunitSensor.java
	sonar-cxx-plugin/src/test/java/org/sonar/plugins/cxx/TestUtils.java
	sonar-cxx-plugin/src/test/java/org/sonar/plugins/cxx/compiler/CxxCompilerSensorTest.java
	sonar-cxx-plugin/src/test/java/org/sonar/plugins/cxx/coverage/CxxCoverageSensorTest.java
	sonar-cxx-plugin/src/test/java/org/sonar/plugins/cxx/cppcheck/CxxCppCheckSensorTest.java
	sonar-cxx-plugin/src/test/java/org/sonar/plugins/cxx/pclint/CxxPCLintSensorTest.java
	sonar-cxx-plugin/src/test/java/org/sonar/plugins/cxx/rats/CxxRatsSensorTest.java
	sonar-cxx-plugin/src/test/java/org/sonar/plugins/cxx/utils/CxxReportSensorTest.java
	sonar-cxx-plugin/src/test/java/org/sonar/plugins/cxx/utils/CxxReportSensor_getReports_Test.java
	sonar-cxx-plugin/src/test/java/org/sonar/plugins/cxx/valgrind/CxxValgrindSensorTest.java
	sonar-cxx-plugin/src/test/java/org/sonar/plugins/cxx/xunit/CxxXunitSensorTest.java
@jmecosta
Copy link
Member Author

@Bertk i had to disable one of your tests. the vc log contains absolute paths that do not correspond to the place where i run the tests. so they fail. can you take a look to see if test data needs to be modified?

…-module-with-reactor

Conflicts:
	sonar-cxx-plugin/src/test/java/org/sonar/plugins/cxx/compiler/CxxCompilerSensorTest.java
@guwirth guwirth mentioned this pull request Apr 17, 2015
@jmecosta
Copy link
Member Author

@guwirth something wrong with Travis some builds are failing while running behave

@guwirth
Copy link
Collaborator

guwirth commented Apr 17, 2015

@jmecosta I had this also twice this week. After triggering again a build with the same sources it worked. I'm not that familiar with Travis. I'm not sure if this is a Travis issue or a problem with our build script.

sonar.host.url=http://localhost:9000

sonar.modules=cli,lib,package1,package2
cli.sonar.projectBaseDir=cli
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are all these settings (cli, doxygen, ...) needed or can they removed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Only doxygen is redundant but its not worth it

On Sat, Apr 18, 2015, 08:57 Günter Wirth [email protected] wrote:

In
integration-tests/testdata/cpp-multimodule-project/sonar-project.properties
#358 (comment):

@@ -0,0 +1,56 @@
+sonar.host.url=http://localhost:9000
+
+sonar.modules=cli,lib,package1,package2
+cli.sonar.projectBaseDir=cli

Are all these settings (cli, doxygen, ...) needed or can they removed?


Reply to this email directly or view it on GitHub
https://github.com/wenns/sonar-cxx/pull/358/files#r28641725.

@jmecosta
Copy link
Member Author

@guwirth @Bertk if all goes well in travis this pull request should be ok. ive enabled the test that ive earlier disable and change the paths in the report to be relative.

Paths there were of type : c:\tfs_server\bla ive patched those one to be relative. (usually this is not a issue because c:\tfs_server is the root of the project) i think this shoud be fine to test the sensor

@guwirth
Copy link
Collaborator

guwirth commented Apr 18, 2015

There were some complaints from @wenns in January, are they solved/answered?

  • the xunit simple mode is not working
  • reports are processed N times (1 would be sufficient)
  • reports are producing lots of wanings of type
    'could not find file ' because this file is in an other module
  • project-level violations will be assigned to each module
    etc.

@jmecosta
Copy link
Member Author

I think some, but I guess improvements can be done later. This initial
support is good enough for multi module users. But is up to you guys.

On Sat, Apr 18, 2015, 09:12 Günter Wirth [email protected] wrote:

There were some complaints from @wenns https://github.com/wenns in
January, are they solved/answered?

  • the xunit simple mode is not working
  • reports are processed N times (1 would be sufficient)
  • reports are producing lots of wanings of type 'could not find file '
    because this file is in an other module
  • project-level violations will be assigned to each module etc.

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

@guwirth
Copy link
Collaborator

guwirth commented Apr 18, 2015

@jmecosta could rebase and squash (combine multiple commits into one) your PR please. I will merge it then for testing.

guwirth added a commit that referenced this pull request Apr 18, 2015
…eactor

Enhancement/multi module with reactor
@guwirth guwirth merged commit 5080011 into SonarOpenCommunity:master Apr 18, 2015
@jmecosta
Copy link
Member Author

You ended up merging, was doing the rebase a bit later. But not now :)

On Sat, Apr 18, 2015, 11:56 Günter Wirth [email protected] wrote:

Merged #358 #358.


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

@guwirth
Copy link
Collaborator

guwirth commented Apr 18, 2015

Sorry don't catch you. Should I redo the merge? Thought is finished for testing?

Am 18.04.2015 um 10:59 schrieb Jorge Costa [email protected]:

You ended up merging, was doing the rebase a bit later. But not now :)

On Sat, Apr 18, 2015, 11:56 Günter Wirth [email protected] wrote:

Merged #358 #358.


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


Reply to this email directly or view it on GitHub.

@jmecosta
Copy link
Member Author

No its fine, you ask to rebase and squash. But you ended up merging as is

On Sat, Apr 18, 2015, 12:03 Günter Wirth [email protected] wrote:

Sorry don't catch you. Should I redo the merge? Thought is finished for
testing?

Am 18.04.2015 um 10:59 schrieb Jorge Costa [email protected]:

You ended up merging, was doing the rebase a bit later. But not now :)

On Sat, Apr 18, 2015, 11:56 Günter Wirth [email protected]
wrote:

Merged #358 #358.


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


Reply to this email directly or view it on GitHub.


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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

4 participants