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

Include parser for Polyspace static analysis tool #1412

Merged
merged 15 commits into from
Jan 24, 2023

Conversation

evahabeeballah
Copy link
Contributor

@evahabeeballah evahabeeballah commented Nov 27, 2022

This pull request is to include a parser for .csv result files generated from Polyspace tool.
This request is linked analysis-model pull request: https://github.com/jenkinsci/analysis-model/pull/863
Link for polyspace static analysis tool: https://de.mathworks.com/products/polyspace.html

  • Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
  • Ensure that the pull request title represents the desired changelog entry
  • Please describe what you did
  • Link to relevant issues in GitHub or Jira
  • Link to relevant pull requests, esp. upstream and downstream changes
  • Ensure you have provided tests - that demonstrates feature works or fixes the issue

@uhafner uhafner marked this pull request as draft November 30, 2022 10:33
Copy link
Member

@uhafner uhafner left a comment

Choose a reason for hiding this comment

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

Requires a release (or incremental version) of https://github.com/jenkinsci/analysis-model-api-plugin before we can merge this change. You can reference that version in the pom.xml if it has been published.

Regarding your questions:

Is it possible to change the names of the severity categories to something else other than Low, Normal, High? For example: Error, Not-Run, Warning, Incomplete.

No this is not possible, all parsers share the same model and the same severities.

Regarding the License for the addition of icons in Warnings-NG, what is expected to be provided if the icon was created based on the logo of the tool (Cropped and resized) ?

You need to use the logo license as starting point. What is the current license? If this license allows modifications that we simply can stick to it. Note: it would be helpful to use an SVG if possible.

@evahabeeballah
Copy link
Contributor Author

Since the changes done on analysis-model have been released, do i also need to do any changes on analysis-model-api for the version or is it updated already?
Is it correct that the most recent version of analysis-model-api that also includes our changes is 10.22.0 (what should be reference in pom.xml of warnings-ng-plugin)?

@uhafner
Copy link
Member

uhafner commented Dec 13, 2022

I updated the pom.xml so you need to rebase and retry.

@codecov
Copy link

codecov bot commented Dec 19, 2022

Codecov Report

Merging #1412 (ac7a808) into master (5ca5a1b) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##             master    #1412      +/-   ##
============================================
+ Coverage     79.80%   79.83%   +0.02%     
- Complexity     1471     1473       +2     
============================================
  Files           252      254       +2     
  Lines          5636     5644       +8     
  Branches        425      425              
============================================
+ Hits           4498     4506       +8     
  Misses          986      986              
  Partials        152      152              
Impacted Files Coverage Δ
...o/jenkins/plugins/analysis/warnings/Polyspace.java 100.00% <100.00%> (ø)
...nkins/plugins/analysis/warnings/SimulinkCheck.java 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@evahabeeballah
Copy link
Contributor Author

Hi,
Regarding the 'Enforce PR labels / enforce-label (pull_request)' check that is failed, can you please let me know what that means, or what can I do to pass the check?

@uhafner uhafner added the enhancement Enhancement of existing functionality label Dec 19, 2022
@uhafner
Copy link
Member

uhafner commented Dec 19, 2022

Hi,
Regarding the 'Enforce PR labels / enforce-label (pull_request)' check that is failed, can you please let me know what that means, or what can I do to pass the check?

This is a check for me so that I do not merge without a label.

}

/** Descriptor for this static analysis tool. */
@Symbol("PolyspaceParse")
Copy link
Member

@uhafner uhafner Dec 19, 2022

Choose a reason for hiding this comment

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

Seems that I missed that detail in my analysis-model PR.

Wouldn't that be a more appropriate symbol?

Suggested change
@Symbol("PolyspaceParse")
@Symbol("polyspaceParser")

Where does the Parse part came from? Is the tool called Polyspace Parse? Or is this suffix solely from your parser?

Either way: symbols need to use CamelCase, IDs need to use url-notation (all lower case). I can fix that right away in the analysis model as well.

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The idea was to differentiate between polyspace commands and the parser command, just in case there is a command from polyspace tool defined in the same way as 'polyspace'.

Copy link
Member

Choose a reason for hiding this comment

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

I see. Then let us use polyspaceParser if that is fine for you as well, since we already have several other tools that do it this way (PMD, AndroidLint).

Copy link
Member

Choose a reason for hiding this comment

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

The appropriate ID would then be polyspace-parser.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes polyspace-parser would be fine then. I will change that now.

Copy link
Member

Choose a reason for hiding this comment

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

In analysis-model as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes also in analysis-model. But you can change that ? Since the pull request is already approved.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, this library is under my control and only used by tools that I control. So no worries...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will also do the same for the Simulink check Parser in the other Pull request

Polyspace icon:

The Polyspace icon is based on tool logo from https://w...mathworks.com/products/polyspace.html
Licensed by The MathWorks, Inc
Copy link
Member

Choose a reason for hiding this comment

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

What is the actual license? Are we allowed to add it to our MIT based project?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After checking the licence file, we are not sure if we can use it this way. I will remove the icon file and push the changes again, will that be okay ?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, this would be more safe. Image licensing is always problematic...

@evahabeeballah evahabeeballah marked this pull request as ready for review January 23, 2023 08:40
@evahabeeballah
Copy link
Contributor Author

Hello, I am not sure what is the problem with this pull request. Can you please let me know if there is anything to be done from my side. Thanks !

@@ -25,7 +25,7 @@
<changelist>-SNAPSHOT</changelist>
<module.name>${project.groupId}.warnings.ng</module.name>

<analysis-model-api.version>10.21.0</analysis-model-api.version>
<analysis-model-api.version>10.22.0</analysis-model-api.version>
Copy link
Member

Choose a reason for hiding this comment

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

I think you simply forgot to update the model version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Enhancement of existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants