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

iOS plugin #5

Merged
merged 47 commits into from
Feb 13, 2023
Merged

iOS plugin #5

merged 47 commits into from
Feb 13, 2023

Conversation

dedece35
Copy link
Member

@dedece35 dedece35 commented Nov 28, 2022

Description

This PR added support to iOS mobile apps (with Swift language support).

In order to work, the plugin needs to be installed alongside a plugin supporting Swift as a language such as sonar-apple (open source) or the commercial version from SonarSource.

This initial version comes with a single rule, but lays the groundwork for implementing (many !) more.
A basic CONTRIBUTION.md explains how to add new rules.

profile

initial PR (before migration): cnumr/ecoCode#146
(plop @zippy1978 )

@dedece35 dedece35 added the ios Something tied to the iOS label Nov 28, 2022
@dedece35 dedece35 mentioned this pull request Nov 28, 2022
@olegoaer olegoaer added the 🚀 enhancement New feature or request label Nov 28, 2022
src/ios-plugin/README.md Outdated Show resolved Hide resolved
src/ios-plugin/README.md Outdated Show resolved Hide resolved
src/ios-plugin/README.md Outdated Show resolved Hide resolved
src/ios-plugin/commons/pom.xml Outdated Show resolved Hide resolved
src/ios-plugin/commons/pom.xml Outdated Show resolved Hide resolved
src/ios-plugin/sonar-ios-plugin/pom.xml Outdated Show resolved Hide resolved
src/ios-plugin/sonar-ios-plugin/pom.xml Outdated Show resolved Hide resolved
src/ios-plugin/swift-lang/pom.xml Outdated Show resolved Hide resolved
@dedece35
Copy link
Member Author

dedece35 commented Dec 3, 2022

Hi @zippy1978,
please take into account my review comments in this PR.

I'm not expert in ios development but I've worked with java and maven since many years. If you want to discuss about my comments on Slack appointment, no problem. It will be a pleasure to talk to you.

Hi @julien-hertout-neomades, @jhertout,
could you give me your test files to check that this new ios plugin is ok ? could we make an appointment on Slack / Meet to discuss about it ?

@dedece35 dedece35 marked this pull request as draft December 3, 2022 22:14
@zippy1978
Copy link
Contributor

Hi @dedece35,
And thanks for your feedbacks.
I will have a look a them and probably get in touch with you when needed.

If possible, I will be interested to join the Slack as well !

@dedece35
Copy link
Member Author

dedece35 commented Dec 4, 2022

hi @zippy1978
no problem to join slack : what's your email slack account to invite you ?

@zippy1978
Copy link
Contributor

@dedece35 gi.grousset@gmail.com :)

Copy link
Member Author

@dedece35 dedece35 left a comment

Choose a reason for hiding this comment

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

with migration of SonarQube from 9.3 to 9.8, some source code must change ...
please check PR #13 on current repo

android-plugin/pom.xml Show resolved Hide resolved
codenarc-converter/pom.xml Show resolved Hide resolved
ios-plugin/pom.xml Show resolved Hide resolved
ios-plugin/pom.xml Outdated Show resolved Hide resolved
ios-plugin/pom.xml Outdated Show resolved Hide resolved
ios-plugin/pom.xml Outdated Show resolved Hide resolved
ios-plugin/sonar-ios-plugin/pom.xml Outdated Show resolved Hide resolved
ios-plugin/sonar-ios-plugin/pom.xml Outdated Show resolved Hide resolved
ios-plugin/sonar-ios-plugin/pom.xml Outdated Show resolved Hide resolved
ios-plugin/docker-compose.yml Outdated Show resolved Hide resolved
Copy link
Member Author

@dedece35 dedece35 left a comment

Choose a reason for hiding this comment

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

Hi @zippy1978,
all is almost good.
please see the few remaining comments .

@jhertout

This comment was marked as resolved.

@zippy1978
Copy link
Contributor

@jhertout @dedece35 ,
I did fix the packaging issue @jhertout had.

And also addressed the requested changes.

For the license header in files: I used this maven plugin (I used it on previous projects) : https://mycila.carbou.me/license-maven-plugin
It can add and check source file headers automatically. Maybe something to add on other plugins as well ?

* SonarQube Apple Plugin - Enables analysis of Swift and Objective-C projects into SonarQube.
* Copyright © 2022 inside|app (contact@insideapp.fr)
* ecoCode iOS plugin - Help the earth, adopt this green plugin for your applications
* Copyright © 2022 CNumR (https://www.ecocode.io/)
Copy link
Member Author

Choose a reason for hiding this comment

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

not CNumR but Green Code Initiative (same review note in all files)

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi,
Actually the header is generated automatically from pom data.
In parent pom.xml organization is still CNumR.
I will update it.
Also url is still https://collectif.greenit.fr, should I change it as well ?

Copy link
Contributor

Choose a reason for hiding this comment

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

We have a dedicated PR for this. We will change it later.

@@ -145,6 +145,35 @@
</executions>
</plugin>

<plugin>
<groupId>com.mycila</groupId>
Copy link
Member Author

Choose a reason for hiding this comment

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

maybe discuss about it during weekly meeting

Copy link
Contributor

@jhertout jhertout left a comment

Choose a reason for hiding this comment

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

Hi @zippy1978,

Thanks for the update. I had be able to install and test the plugin. I have some last (I hope :)) remarks:

  • In the docker-compose in the ios-plugin folder, can you remove the line "- "extensions:/opt/sonarqube/extensions". It does not work with it. It was a patch we did for a deployment in our Snapp' gitlab at the begining of the project. It is not required anymore.
  • There is no "tags" in your rule description file. Can you add "ecocode, environment, idleness" as tags for the rule?
  • Can you be a little more complete in the rule detail description: add why the rule is a bad practice by explaining what the original call does and why it is energy consuming. You can add the code "compliant" and "not compliant" samples too (only bad if necessary).
  • I create a swift project to test the plugin (its aim is to have all the issues in a swift project and to see if they are detected by the plugin): https://github.com/green-code-initiative/ecoCode-mobile-ios-swift-test-project. When I perform an analyse using the sonar-scanner (see project readme), the analyse succeeds and the plugin works but I have exceptions during the scanner work:

scanner-error

Do you have an idea of what the problem is?

I notice that the ios-plugin deployment does not work with the "global" docker-file but we will fix it later. It is not important if the project works only in the "ios-plugin environment" for the moment.

Thanks

@zippy1978
Copy link
Contributor

@jhertout

Regarding the stacktrace: it is not related to the ecoCode plugin but to the Swift lang support plugin (that is required) and that we have been developing at inside|app here: https://github.com/insideapp-oss/sonar-apple.

So we did some investigation, and it is probably caused by a mix in line ending chars in the source files.
I think you did test on Windows, and your git client is probably configured to adapt line ending on checkout (and use CR+LF chars, instead of leaving LF - as used on macOS and Linux) as a result SonarQube cannot detect lines in source files.

On Windows this feature is called "autocrlf".

This problem is SonarQube related (not specific / not related to specific plugin) and can happen with any language (see https://stackoverflow.com/questions/43997878/sonarqube-c-sharp-analysis-fails-to-not-a-valid-line-offset-for-pointer)

Note that the issue does not appear on macOS and Linux (using your test project + running SonarQube in docker-compose).

Could you double check the line ending chars on your test project sources ?

@sonarqubecloud
Copy link

sonarqubecloud bot commented Feb 3, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

80.8% 80.8% Coverage
0.0% 0.0% Duplication

@zippy1978
Copy link
Contributor

Hi @jhertout,

I addressed all the points you mentioned above.

The rule now looks like:

Capture d’écran 2023-02-03 à 23 29 56

@jhertout
Copy link
Contributor

jhertout commented Feb 7, 2023

Hello @zippy1978 ,

it seems ok for me. I don't remember if I answer you relating the stacktrace I sent to you but you were right. The problem was that I work on Windows and my EOL was changed by git.
I will try to perform a last test this week. If all is ok, I will merge this PR and we will follow the work in other PRs if necessary.

Thanks

@jhertout jhertout marked this pull request as ready for review February 13, 2023 14:03
@jhertout jhertout merged commit 64c189a into main Feb 13, 2023
@dedece35 dedece35 deleted the insideapp-oss_feature_swift-plugin branch February 25, 2023 21:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ios Something tied to the iOS 🚀 enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants