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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: Upgrade to SonarQube 7.6 #152

Merged
merged 18 commits into from Mar 10, 2019
Merged

Conversation

@BalmungSan
Copy link
Contributor

BalmungSan commented Feb 15, 2019

Good news: Everything compiles 馃帀
Bad news: A couple of deprecation warnings! 馃檧 - Apparently they removed the concept of modules, thus the changes may be heavy, I have not looked to them yet, hopping to start working in this latter today, or tomorrow.

Anyways, at this moment I can not test this with a real SonarQube server.
Maybe @manodupont or @stillleben can help out with that part.
It would be good to test now and after removing the deprecations - on the worst case, if I can not fix them in the short term, we may merge this as is, release (if everything works) and fix the deprecations latter.


Edit

Real bad new!: I only checked if it compiled, but did not run the tests. 馃槥 - Apparently, since the tests uses the internal API those are broken (don't even compile). Sorry about that.

@mwz

This comment has been minimized.

Copy link
Owner

mwz commented Feb 15, 2019

Thanks for looking into this @BalmungSan - perhaps it would be easier to try to upgrade to 7.5 first?

@BalmungSan

This comment has been minimized.

Copy link
Contributor Author

BalmungSan commented Feb 15, 2019

"Perhaps it would be easier to try to upgrade to 7.5 first?"

May not be 100% true, but I think not.
The release notes state that 7.5 only added support for Scala and Apex, and some other graphical changes. Thus, I do not think it will make any impact.

馃憤

@mwz

This comment has been minimized.

Copy link
Owner

mwz commented Feb 15, 2019

Ok no problem - let me know if you need any help with this and I can give it a go if it turns out that it's too much work.

BalmungSan added 5 commits Feb 17, 2019
@BalmungSan

This comment has been minimized.

Copy link
Contributor Author

BalmungSan commented Feb 17, 2019

Hi @mwz, addressed the deprecations and fixed the tests...

However, I still believe this will not work... at least not for multi-module projects.
At this moment I can not run the examples against a running SonarQube server, could you help me with that?

I had been searching and Scoverage supports generation of aggregated metrics (also the sunfire maven plugin), however I can not say the same for Scapegoat and Scalastyle 馃樋 ...
Maybe, we could try to aggregate the reports ourselves?
Other option, would be to check if using the old Sensor instead of the new ProjectSensor will made them work on modules again, and see if SonarQube will be able to aggregate the metrics. Can you help me testing this one too?

PS: Also, there are a lot of uses of the word module on tests folder, I will try to fix that tomorrow.

@mwz

This comment has been minimized.

Copy link
Owner

mwz commented Feb 22, 2019

Sorry, @BalmungSan I haven't yet had a chance to look into this, but hopefully, I should be able to spend some time on this over the weekend.

@BalmungSan

This comment has been minimized.

Copy link
Contributor Author

BalmungSan commented Feb 23, 2019

@mwz no problem, my week has been very busy too. I would not had been able to address any feedback 馃憤

log.debug(s"Input test files: \n${inputFiles.mkString(", ")}")
else
log.warn(s"No test files found for module ${context.module.key}.")

This comment has been minimized.

Copy link
@mwz

mwz Feb 24, 2019

Owner

Do you think we don't need this anymore?

This comment has been minimized.

Copy link
@BalmungSan

BalmungSan Feb 24, 2019

Author Contributor

Oh, sorry. Forgot to ask, and end up deleting it anyways.
I did not understand what was the intended use of that.
The inputFile are only used for the logs, and I remember there were some similar logs, like empty test directories.

Care to explain what was the use of this?, If you consider it important I can add it again.

This comment has been minimized.

Copy link
@BalmungSan

BalmungSan Mar 2, 2019

Author Contributor

@mwz So, should I restore this?

This comment has been minimized.

Copy link
@mwz

mwz Mar 2, 2019

Owner

Yes, I think so. This will log warnings when we don't find any test files in the module, so it's useful - a gentle reminder that you should have tests in your project ;)

@mwz

This comment has been minimized.

Copy link
Owner

mwz commented Feb 24, 2019

I've tried executing an analysis against the plugin built from your branch, but I'm getting the following dependency injection error at runtime:

ERROR: Error during SonarQube Scanner execution
org.picocontainer.injectors.AbstractInjector$UnsatisfiableDependenciesException: com.mwz.sonar.scala.scalastyle.ScalastyleSensor has unsatisfied dependency 'class org.sonar.api.profiles.RulesProfile' for constructor 'public com.mwz.sonar.scala.scalastyle.ScalastyleSensor(org.sonar.api.profiles.RulesProfile,com.mwz.sonar.scala.scalastyle.ScalastyleCheckerAPI)' from org.sonar.core.platform.ComponentContainer$ExtendedDefaultPicoContainer@740fb309:307<[Immutable]:org.sonar.core.platform.ComponentContainer$ExtendedDefaultPicoContainer@771158fb:33<|
	at org.picocontainer.injectors.ConstructorInjector.getGreediestSatisfiableConstructor(ConstructorInjector.java:191)
	at org.picocontainer.injectors.ConstructorInjector.getGreediestSatisfiableConstructor(ConstructorInjector.java:110)
	at org.picocontainer.injectors.ConstructorInjector.access$100(ConstructorInjector.java:51)
	at org.picocontainer.injectors.ConstructorInjector$1.run(ConstructorInjector.java:331)
	at org.picocontainer.injectors.AbstractInjector$ThreadLocalCyclicDependencyGuard.observe(AbstractInjector.java:270)
	at org.picocontainer.injectors.ConstructorInjector.getComponentInstance(ConstructorInjector.java:364)
	at org.picocontainer.injectors.AbstractInjectionFactory$LifecycleAdapter.getComponentInstance(AbstractInjectionFactory.java:56)
	at org.picocontainer.behaviors.AbstractBehavior.getComponentInstance(AbstractBehavior.java:64)
	at org.picocontainer.behaviors.Stored.getComponentInstance(Stored.java:91)
	at org.picocontainer.DefaultPicoContainer.getLocalInstance(DefaultPicoContainer.java:606)
	at org.picocontainer.DefaultPicoContainer.getComponents(DefaultPicoContainer.java:587)
	at org.sonar.core.platform.ComponentContainer.getComponentsByType(ComponentContainer.java:290)
	at org.sonar.scanner.bootstrap.AbstractExtensionDictionnary.completeScannerExtensions(AbstractExtensionDictionnary.java:82)
	at org.sonar.scanner.bootstrap.AbstractExtensionDictionnary.getExtensions(AbstractExtensionDictionnary.java:77)
	at org.sonar.scanner.bootstrap.AbstractExtensionDictionnary.getFilteredExtensions(AbstractExtensionDictionnary.java:67)
	at org.sonar.scanner.sensor.ProjectSensorExtensionDictionnary.selectSensors(ProjectSensorExtensionDictionnary.java:41)
	at org.sonar.scanner.sensor.ProjectSensorsExecutor.execute(ProjectSensorsExecutor.java:41)
	at org.sonar.scanner.scan.ProjectScanContainer.doAfterStart(ProjectScanContainer.java:363)
	at org.sonar.core.platform.ComponentContainer.startComponents(ComponentContainer.java:136)
	at org.sonar.core.platform.ComponentContainer.execute(ComponentContainer.java:122)
	at org.sonar.scanner.bootstrap.GlobalContainer.doAfterStart(GlobalContainer.java:126)
	at org.sonar.core.platform.ComponentContainer.startComponents(ComponentContainer.java:136)
	at org.sonar.core.platform.ComponentContainer.execute(ComponentContainer.java:122)
	at org.sonar.batch.bootstrapper.Batch.doExecute(Batch.java:73)
	at org.sonar.batch.bootstrapper.Batch.executeTask(Batch.java:99)
	at org.sonarsource.scanner.api.internal.batch.BatchIsolatedLauncher.execute(BatchIsolatedLauncher.java:63)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)
	at org.sonarsource.scanner.api.internal.IsolatedLauncherProxy.invoke(IsolatedLauncherProxy.java:60)
	at com.sun.proxy.$Proxy0.execute(Unknown Source)
	at org.sonarsource.scanner.api.EmbeddedScanner.doExecute(EmbeddedScanner.java:233)
	at org.sonarsource.scanner.api.EmbeddedScanner.runAnalysis(EmbeddedScanner.java:151)
	at org.sonarsource.scanner.cli.Main.runAnalysis(Main.java:123)
	at org.sonarsource.scanner.cli.Main.execute(Main.java:77)
	at org.sonarsource.scanner.cli.Main.main(Main.java:61)

I think that's caused by the RulesProfile no longer being available for scanner side components (as hinted in the API changes the ActiveRules should be used instead)

@mwz

This comment has been minimized.

Copy link
Owner

mwz commented Feb 24, 2019

So I had a brief look at the recent SQ changes and it sounds to me that they removed the module functionality from the UI and deprecated some of the batch APIs, but everything should work for now (except for the removed APIs for RulesProfile which is what we use and scanner tasks which we don't use).

I think we should still try to scan using modules because as far as I understand this functionality should still work along with scanner compatibility and while they are phasing modules out we can come up with a longer term plan on how we can do aggregation on our end (if it's necessary and not taken care by the "new scanner engine").

I'm going to test against 7.6 with those deprecated APIs and the latest scanner to check if things are still working - the best case scenario is we'll only need to fix the RulesProfile breaking change.

@BalmungSan

This comment has been minimized.

Copy link
Contributor Author

BalmungSan commented Feb 24, 2019

Hi, will try to finish this and fix the dependency injection problem today or tomorrow.

"The best case scenario is we'll only need to fix the RulesProfile breaking change".

I hope so - will stay tuned for your update.

"I think we should still try to scan using modules because as far as I understand this functionality should still work along with scanner compatibility".

If it works, I agree we should do so for Scapegoat and Scalastyle, but if Scoverage & JUnit already support generation of aggregated reports, I think (IMHO) we should prefer that. - What do you think.

@mwz

This comment has been minimized.

Copy link
Owner

mwz commented Feb 25, 2019

I've made the necessary changes on a separate branch (see here) and everything seems to be working fine although it might require some more testing as I've checked only the single and multi-module example projects. (I haven't fixed Scapegoat tests, but I did that for Scalastyle).

Feel free to use my branch as a reference.

These are the only warnings I get from the scanner:

WARN: Storing measures on folders or modules is deprecated. Provided value of metric 'sonar-scala-scoverage-total-statements' is ignored.
WARN: Storing measures on folders or modules is deprecated. Provided value of metric 'sonar-scala-scoverage-covered-statements' is ignored.
WARN: Storing measures on folders or modules is deprecated. Provided value of metric 'sonar-scala-scoverage-statement-coverage' is ignored.
WARN: Storing measures on folders or modules is deprecated. Provided value of metric 'sonar-scala-scoverage-branch-scoverage' is ignored.

I don't think we need to worry about those for now - I've checked and all the coverage metrics are saved correctly. I'm not sure why we're getting those errors - I don't think we save any of those metrics on either module or folders as we do that on a file level... Maybe it's something we can look into later.

Hi, will try to finish this and fix the dependency injection problem today or tomorrow.

Looks like we've used RulesProfile to get the name of the default quality profile (here) so I got rid of it entirely.

If it works, I agree we should do so for Scapegoat and Scalastyle, but if Scoverage & JUnit already support generation of aggregated reports, I think (IMHO) we should prefer that. - What do you think.

I'd generally agree with that, however we'd need to be careful not to confuse people as to which features use aggregated reports and which don't. It would be great if we could be consistent and base everything on aggregated reports, but as you said before Scapegoat doesn't support this, so most likely to implement a project level sensor we would need users to define source directories of their modules in sonar.sources property (sbt-sonar could do that), generate scapegoat report for each module and during the analysis we would load and process all of the available reports and maybe for consistency reasons we should do the same with scoverage and junit. It would be nice as a user not having to worry about aggregation and run any additional tasks. With Scalastyle we have a lot more controll because we can just run it for each directory with sources, so that sould be fairly straightforward.

We should definitely discuss it in more details and agree on the approach for next versions. For 7.6 though I'd be happy to go with those deprecated APIs and a minimal set of changes given that everything still works.

@mwz

This comment has been minimized.

Copy link
Owner

mwz commented Mar 1, 2019

Hey @BalmungSan, let me know if you want me to pick this up.

@BalmungSan

This comment has been minimized.

Copy link
Contributor Author

BalmungSan commented Mar 2, 2019

Hi @mwz sorry, this week was even worse than the last one. I hope everything will finish soon and after that I will have some spare time, I really had not even read your previous comment... 馃槥

I should be able to resume this before Monday, but if you wan to improve something meanwhile or close this PR and open one yourself, I would be fine.

@mwz

This comment has been minimized.

Copy link
Owner

mwz commented Mar 2, 2019

I should be able to resume this before Monday, but if you wan to improve something meanwhile or close this PR and open one yourself, I would be fine.

Ok, no worries - I'll leave this to you then.

@BalmungSan

This comment has been minimized.

Copy link
Contributor Author

BalmungSan commented Mar 2, 2019

"We should definitely discuss it in more details and agree on the approach for next versions. For 7.6 though I'd be happy to go with those deprecated APIs and a minimal set of changes given that everything still works".

I agree we should discuss this in more detailed way latter (probably we should open a new Issue for tracking that #165 ).

I somewhat disagree with your idea, but I'll keep my thoughts for now, and we will discuss it latter 馃槈
For now I will leave everything working in a module level, but will remove everything that says "module".

Are you ok with that?

@BalmungSan

This comment has been minimized.

Copy link
Contributor Author

BalmungSan commented Mar 4, 2019

Hi @mwz, I think I am done here. 馃
let me know if everything works as expected or if you have any suggestion.

After this gets merged, I will post in #165 my thoughts about how I believe the plugin should work on future releases.

@mwz

This comment has been minimized.

Copy link
Owner

mwz commented Mar 4, 2019

Thanks @BalmungSan, the changes look good to me. I'll test this locally and if everything is good I'll merge it and make a release.

@mwz

This comment has been minimized.

Copy link
Owner

mwz commented Mar 6, 2019

I've just tested this against the example multi-module sbt project and this is the error I got during the analysis of a second module:

ERROR: Error during SonarQube Scanner execution
java.lang.UnsupportedOperationException: Can not add the same measure twice on [key=example-sbt-multi-module]: DefaultMeasure[component=[key=example-sbt-multi-module],metric=Metric[id=,key=sonar-scala-scoverage-total-statements,description=Number of all statements,type=INT,direction=1,domain=Size,name=Total statements,qualitative=false,userManaged=false,enabled=true,worstValue=,bestValue=,optimizedBestValue=false,hidden=false,deleteHistoricalData=false,decimalScale=],value=17,fromCore=false]
at org.sonar.scanner.sensor.DefaultSensorStorage.saveMeasure(DefaultSensorStorage.java:298)
at org.sonar.scanner.sensor.DefaultSensorStorage.store(DefaultSensorStorage.java:236)
at org.sonar.api.batch.sensor.measure.internal.DefaultMeasure.doSave(DefaultMeasure.java:95)
at org.sonar.api.batch.sensor.internal.DefaultStorable.save(DefaultStorable.java:45)
at com.mwz.sonar.scala.scoverage.ScoverageSensor.saveComponentScoverage(ScoverageSensor.scala:136)
at com.mwz.sonar.scala.scoverage.ScoverageSensor.execute(ScoverageSensor.scala:67)
at org.sonar.scanner.sensor.AbstractSensorWrapper.analyse(AbstractSensorWrapper.java:48)
at org.sonar.scanner.sensor.ModuleSensorsExecutor.execute(ModuleSensorsExecutor.java:85)
at org.sonar.scanner.sensor.ModuleSensorsExecutor.lambda$execute$1(ModuleSensorsExecutor.java:59)
at org.sonar.scanner.sensor.ModuleSensorsExecutor.withModuleStrategy(ModuleSensorsExecutor.java:77)
at org.sonar.scanner.sensor.ModuleSensorsExecutor.execute(ModuleSensorsExecutor.java:59)
at org.sonar.scanner.scan.ModuleScanContainer.doAfterStart(ModuleScanContainer.java:82)
at org.sonar.core.platform.ComponentContainer.startComponents(ComponentContainer.java:136)
at org.sonar.core.platform.ComponentContainer.execute(ComponentContainer.java:122)
at org.sonar.scanner.scan.ProjectScanContainer.scan(ProjectScanContainer.java:408)
at org.sonar.scanner.scan.ProjectScanContainer.scanRecursively(ProjectScanContainer.java:403)
at org.sonar.scanner.scan.ProjectScanContainer.scanRecursively(ProjectScanContainer.java:400)
at org.sonar.scanner.scan.ProjectScanContainer.doAfterStart(ProjectScanContainer.java:360)
at org.sonar.core.platform.ComponentContainer.startComponents(ComponentContainer.java:136)
at org.sonar.core.platform.ComponentContainer.execute(ComponentContainer.java:122)
at org.sonar.scanner.bootstrap.GlobalContainer.doAfterStart(GlobalContainer.java:126)
at org.sonar.core.platform.ComponentContainer.startComponents(ComponentContainer.java:136)
at org.sonar.core.platform.ComponentContainer.execute(ComponentContainer.java:122)
at org.sonar.batch.bootstrapper.Batch.doExecute(Batch.java:73)
at org.sonar.batch.bootstrapper.Batch.executeTask(Batch.java:99)
at org.sonarsource.scanner.api.internal.batch.BatchIsolatedLauncher.execute(BatchIsolatedLauncher.java:63)
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.lang.reflect.Method.invoke(Method.java:498)
at org.sonarsource.scanner.api.internal.IsolatedLauncherProxy.invoke(IsolatedLauncherProxy.java:60)
at com.sun.proxy.$Proxy0.execute(Unknown Source)
at org.sonarsource.scanner.api.EmbeddedScanner.doExecute(EmbeddedScanner.java:233)
at org.sonarsource.scanner.api.EmbeddedScanner.runAnalysis(EmbeddedScanner.java:151)
at org.sonarsource.scanner.cli.Main.runAnalysis(Main.java:123)
at org.sonarsource.scanner.cli.Main.execute(Main.java:77)
at org.sonarsource.scanner.cli.Main.main(Main.java:61)


saveComponentScoverage(context, context.module(), moduleCoverage.moduleScoverage)
saveComponentScoverage(context, context.project, projectCoverage.projectScoverage)

This comment has been minimized.

Copy link
@mwz

mwz Mar 6, 2019

Owner

You might still need to save this against the current module, otherwise the scanner explodes.

This comment has been minimized.

Copy link
@BalmungSan

BalmungSan Mar 6, 2019

Author Contributor

Uhm.. the problem is that module is deprecated and the replacement is project.
And, I am not talking about the big picture about module vs project analysis, but about the context methods...

And I thought I understood that using the old context would be enough...
Because for the plugin each module would be seen as a project and that the server will aggregate the metrics...

But, apparently, I was wrong.
Can you please check if using the deprecated module solves this problem?
Other option would be to remove module metrics, and only register the files.

This comment has been minimized.

Copy link
@mwz

mwz Mar 7, 2019

Owner

module is deprecated but it doesn't mean that it doesn't work any more - it will be going away at some point but it still works fine in 7.6. I tested the changes on my branch here and as you can see from the diff I didn't change any module to project.

This comment has been minimized.

Copy link
@BalmungSan

BalmungSan Mar 7, 2019

Author Contributor

@mwz Oh yeah, I know it still works (that is the propose of deprecating things anyways lol).

I mean that, IMO (or what I had understood from the docs), the idea was that if we used the old (almost deprecated) Plugin, instead of the new ProjectPlugin, the project would be the module.

But apparently it does not. - And, since if you said it works, I will revert the changes.

Do you want me to revert it everywhere (a couple of logs)?, or just there?

Also, since the are no more "modules" at interface level, how are these metrics displayed? - Maybe, they just get lost? That is why I suggested just removing the saveMetrics step for the modoule / project (or whatever) until this gets solved.

This comment has been minimized.

Copy link
@mwz

mwz Mar 9, 2019

Owner

And, since if you said it works, I will revert the changes.

Do you want me to revert it everywhere (a couple of logs)?, or just there?

Great, thanks. I don't mind the logs that much, I'm ok to leave them as they are.

Also, since the are no more "modules" at interface level, how are these metrics displayed?

I'm not sure what happens to those - last time I tested it I might have seen some warning regarding those - I'll check again. I though those contributed to the overall metrics for the whole projects but it might not be the case anymore.

This comment has been minimized.

Copy link
@BalmungSan

BalmungSan Mar 9, 2019

Author Contributor

Ok. So for now, I will just revert the changes here.
Will commit and push in a couple of minutes.

Done!
Hope this time everything works.

BalmungSan and others added 4 commits Mar 9, 2019
@mwz

This comment has been minimized.

Copy link
Owner

mwz commented Mar 10, 2019

Ok, I'll merge this and check the scanner warnings for module metrics tomorrow - maybe, as you said, we can remove them if they are not used anywhere.

@mwz mwz merged commit c3d5f8a into mwz:master Mar 10, 2019
1 check passed
1 check passed
ci/circleci: test Your tests passed on CircleCI!
Details
@BalmungSan BalmungSan deleted the BalmungSan:upgrade-to-sonarqube-7.6 branch Mar 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can鈥檛 perform that action at this time.