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

Allow multiple ruleset files #51

Merged
merged 4 commits into from
Sep 28, 2018

Conversation

pkrall520
Copy link
Contributor

Add the ability to chain ruleset files together from the project preferences.

Phillip Krall added 4 commits July 10, 2018 21:32
Add the ability to chain ruleset files together from the project preferences.
Copy link
Member

@adangel adangel left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! I need to think about this a bit further and I would like to know more about your use case:

  • Do you use PMD also outside of eclipse? (e.g. maven, gradle, ant, ...)
  • Do you use the same ruleset? And where do you store the ruleset?
  • Do you know, that a ruleset can reference other rules and even rulesets? E.g. https://pmd.github.io/pmd-6.6.0/pmd_userdocs_making_rulesets.html#bulk-adding-rules
    This might be an alternative solution for you. However, I don't know, whether this works well with what the pmd-eclipse-plugin does...

Thanks for your time!

@@ -106,7 +107,7 @@
* @return Returns the resolved RuleSet File suitable for loading a rule
* set.
*/
File getResolvedRuleSetFile() throws PropertiesException;
List<File> getResolvedRuleSetFile() throws PropertiesException;
Copy link
Member

Choose a reason for hiding this comment

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

Since this returns now a List, I would expect a plural in the naming of the method, such as getResolvedRuleSetFiles().

// Fall back to below
}
List<File> files = new ArrayList<File>();
for (String ruleSetFile : getRuleSetFile().split(",")) {
Copy link
Member

Choose a reason for hiding this comment

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

This seems to be backwards compatible, which is good and important - so we don't change the format of the project properties file.

@pkrall520
Copy link
Contributor Author

@adangel Thanks for taking a look at this. Please find my answers below.

  • Do you use PMD also outside of eclipse? (e.g. maven, gradle, ant, ...)-
    Yes. We use it with ant and Jenkins. I know that Ant and be passed multiple ruleset files and it seems to work as I would expect it to work. The plugin doesn't seem to work that way since it doesn't take more than one file.

  • Do you use the same ruleset? And where do you store the ruleset?
    We have a ton of projects which all inherit from a a few center locations. I would like to have a base ruleset file under tools/pmd/ruleset.xml. This file should hold all of the rules my company cares about and each project should be able to inherit that file. Then they could override it or exclude ones that does not make since for the project.
    See picture below for a better explanation.

  • Do you know, that a ruleset can reference other rules and even rulesets? E.g. https://pmd.github.io/pmd-6.6.0/pmd_userdocs_making_rulesets.html#bulk-adding-rules
    This might be an alternative solution for you. However, I don't know, whether this works well with what the pmd-eclipse-plugin does...

    Yes I have seen this; however, it doesn't seem to work as I would expect it to. Mainly for the use case above. It doesn't seem to work like this for this plugin.

My company would like to start spinning up custom PMD rules around security, but we would also like to have the default ones from the PMD app. It doesn't seem that we can spin up another project and include that new ruleset in the tools folder's ruleset.xml.

image

@regrog
Copy link

regrog commented Sep 17, 2018

I was looking for this feature!
I'm using PMD via Gradle with different ruleSetFiles.
I need to split in different files because I need to provide different exclude-pattern for different rules.
This works fine with Gradle but not with the eclipse plugin.

ruleSetFiles = files("$rootProject.projectDir/config/pmd/pmd.xml",
				"$rootProject.projectDir/config/pmd/pmd01.xml",
				...)

https://docs.gradle.org/current/dsl/org.gradle.api.plugins.quality.Pmd.html#org.gradle.api.plugins.quality.Pmd:ruleSetFiles

Will this PR be merged soon?
Thanks

@adangel
Copy link
Member

adangel commented Sep 19, 2018

Thanks for following up. I'll have a closer look next week (want to do some tests first).

@regrog
Copy link

regrog commented Sep 28, 2018

If you need and example, use this: projectA.zip

Here you have a Gradle project projectA with 2 subprojects: api.
In projectA.gradle you can find 2 rule sets under pmd task configuration (line 25):

  1. config/pmd/pmd.xml
  2. config/pmd/useUtilityClass.xml

pmd.xml adds a specific rule category/java/codestyle.xml/ClassNamingConventions and all rules from category/java/design.xml but excludes UseUtilityClass. There is also a specific exclude-pattern
useUtilityClass.xml adds specific exclude-pattern for this rule

My target here is to disable UseUtilityClass rule for second package and disable all rules defined in pmd.xml for third package.
third package is only checked for UseUtilityClass rule.

running from projectA folder ./gradlew pmdMain you should have 3 warnings from PMD.

versions

Gradle 4.10.2
PMD 6.7.0
java 1.8
buildship 2.2.1

you need buildship to import this into eclipse.

@adangel adangel self-assigned this Sep 28, 2018
@adangel adangel merged commit 7ade1db into pmd:master Sep 28, 2018
adangel added a commit that referenced this pull request Sep 28, 2018
@adangel
Copy link
Member

adangel commented Sep 28, 2018

Hi @regrog ,
I've merged this PR, so it is possible now to select multiple rulesets.
Would you mind testing it?
You can get the zipped update-site from here: https://dl.bintray.com/pmd/pmd-eclipse-plugin/snapshots/zipped/net.sourceforge.pmd.eclipse.p2updatesite-4.0.18.v20180928-1702.zip
(using the normal snapshot update site https://dl.bintray.com/pmd/pmd-eclipse-plugin/snapshots/updates/ is very slow unfortunately due to too many snapshot versions...)

@pkrall520
Copy link
Contributor Author

pkrall520 commented Sep 28, 2018 via email

@regrog
Copy link

regrog commented Oct 1, 2018

Damn, I think this doesn't solve my case.
If I am right, all ruleset files selected are not collected in a complex structure with different exlude-pattern.
If I import rules from my previous example (projectA), the result is that only the exclude pattern from the first file is used .*/src/main/java/third/.*
Unfortunately third package is totally ignored, but it should be checked for category/java/design.xml/UseUtilityClass rule.

My rules:
pmd.xml

<?xml version="1.0" encoding="UTF-8"?>
<ruleset xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
    name="PMD Custom Ruleset"
    xmlns="http://pmd.sf.net/ruleset/1.0.0"
    xsi:noNamespaceSchemaLocation="http://pmd.sf.net/ruleset_xml_schema.xsd"
    xsi:schemaLocation="http://pmd.sf.net/ruleset/1.0.0 
                        http://pmd.sf.net/ruleset_xml_schema.xsd" >

    <description>Rules</description>

    <exclude-pattern>.*/src/main/java/third/.*</exclude-pattern>

    <rule ref="category/java/codestyle.xml/ClassNamingConventions" />

    <rule ref="category/java/design.xml" >
        <exclude name="UseUtilityClass"/>
    </rule>

</ruleset>

useUtilityClass.xml

<?xml version="1.0" encoding="UTF-8"?>
<ruleset xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
    name="Custom Ruleset"
    xmlns="http://pmd.sf.net/ruleset/1.0.0"
    xsi:noNamespaceSchemaLocation="http://pmd.sf.net/ruleset_xml_schema.xsd"
    xsi:schemaLocation="http://pmd.sf.net/ruleset/1.0.0 
                        http://pmd.sf.net/ruleset_xml_schema.xsd" >

    <description>Custom rule</description>
    
    <exclude-pattern>.*/src/main/java/second/.*</exclude-pattern>

    <rule ref="category/java/design.xml/UseUtilityClass" />
</ruleset>

@adangel
Copy link
Member

adangel commented Oct 1, 2018

@regrog I assume, merging both rulesets won't help either?
You are right, we only take the first ruleset completely (with all the exclude-patterns) and just take the rules from the remaining rulesets. We could merge the exclude-patterns as well, but that would mean in your example, that both src/main/java/third and src/main/java/second is excluded for all rules...

You could maybe use Suppressions, to ignore certain rules for some classes, e.g.

<rule ref="category/java/design.xml/UseUtilityClass">
      <properties>
          <property name="violationSuppressXPath" value="//PackageDeclaration/Name[matches(@Image, '^second.')]"/>
      </properties>
</rule>

But that's more a workaround.

Have you thought about moving third and second into a separate project? Since you use anyway different configurations, it sounds like, these two packages are really something different and should live in projects of their own.

@regrog
Copy link

regrog commented Oct 2, 2018

projectA is just an example to explain my situation.
In Gradle all my rules are centralized in the main project and shared with all subprojects (more than 100) with only few exclusions (auto generated source files and few classes). All projects are connected and the rules should be the same to create the same ecosystem.

I assume, merging both rulesets won't help either?

Merge is not the right solution IMHO.
I do not know what Gradle does deeply. I supposed that pmd normally works with separate rulesets.

Here is the task configured by Gradle with multiple rulesets: PmdInvoker.groovy

@pkrall520
Copy link
Contributor Author

@adangel @regrog Sorry I am coming into this a little late. It sounds like the solution you are looking for is to process each reset separately; however, you want to keep them all connected at the same project. Effectively saying PMD run rulesetA on codeA then PMD run rulesetB on codeA and give give me the results for both. Is this correct?

If this is the case, can we just add a checkbox to the properties field ata project level and tell it to process both ruleset as explained above. Just running the PMD Review code N number of times? I think there would be crazy issues if you repeated or changed ruleset values from A to B. We would probably want to tell the user not to do that with some type of pre-process.

@regrog
Copy link

regrog commented Oct 2, 2018

Effectively saying PMD run rulesetA on codeA then PMD run rulesetB on codeA and give give me the results for both. Is this correct?

Yes correct.
However I'm not familiar enough with PMD internal logic to say "run the PMD Review code N number of times", maybe there is a better solution and that's why I looked inside Gradle code.
Let's say, I have a folder with a bunch of rulesets to use in different projects in the same ecosystem with priorities, excluded rules and some specific exclude-pattern associated with a specific rule.
Reference these bunch of rulesets in each project is enough for me.

@pkrall520
Copy link
Contributor Author

As a work around, you could tell gradle to run rulesetA on codeA and pipe that to a pmd-results.xml file then do the same for rulesetB and just combine the XML files.

This doesn't really help in terms of the eclipse plugin, but it sounds like you are just running gradle and seeing the results.

@regrog
Copy link

regrog commented Oct 2, 2018

Gradle works great with multiple rulesets.
I'm trying to achieve the same with eclipse plugin, as @adangel said if you merge all rules, you also merge exclude-pattern that is incorrect for the final result.
The rulesets applied singularly do their job, but in eclipse they are merged together and exclude-pattern is not honored correctly for each rulesets.

@pkrall520
Copy link
Contributor Author

ok. I understand then. I think I stand by my proposal to add the feature to enable what you are talking about. I think we will need to come up with naming and such around it, but I think it is possible. I think you suffer performance because we would need to run pmd from each ruleset since the plugin just calls into the PMD library and as stated if one rule allowed something and other did not allow it, we might hit some odd issues.

@adangel What are your thoughts on this? Does this seem like the direction you are going with the plugin? Would something like this be better added to the PMD library itself? I am happy to build this if you like.

@adangel
Copy link
Member

adangel commented Oct 5, 2018

@regrog I had a look into your sample project and gradle - and there is nothing special about it: Executing multiple rulesets with plain PMD already works, e.g. you can execute PMD on your sample project with that commandline:

pmd-bin-6.7.0/bin/run.sh pmd -d api/src/main/java -f xml -R config/pmd/pmd.xml,config/pmd/useUtilityClass.xml

And you get the correct result - so each ruleset is applied correctly.

@pkrall520
Thinking this over, I believe, this should be the default behavior of the pmd-eclipse-plugin as well.
Therefore we don't need a flag to enable this or disable this functionality.

Currently the plugin deals with a single RuleSet (projectProperties.setProjectRuleSet(...)) and we would need to change this to take a RuleSets (currently, we get a RuleSets instance, take the first ruleset out of it, pass it along to the project properties and when we are about to execute PMD, we wrap this single ruleset again into a RuleSets instance...).

If you are up to it, I'd be happy about a PR :)

@adangel adangel mentioned this pull request Oct 5, 2018
@pkrall520
Copy link
Contributor Author

Sorry for the late reply. Yes I would be happy to help out.

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

Successfully merging this pull request may close these issues.

None yet

3 participants