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.

Sign up for GitHub

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

[MPMD-399] Incorrect warning: The project X does not seem to be compi… #154

Merged
merged 1 commit into from
Jul 7, 2024
Merged

[MPMD-399] Incorrect warning: The project X does not seem to be compi… #154

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions src/main/java/org/apache/maven/plugins/pmd/AbstractPmdReport.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ public abstract class AbstractPmdReport extends AbstractMavenReport {
private File[] excludeRoots;

/**
* Run PMD on the tests.
* Run PMD on the tests as well.
*
* @since 2.2
*/
Expand All @@ -152,7 +152,8 @@ public abstract class AbstractPmdReport extends AbstractMavenReport {
*
* @since 2.2
* @deprecated since 3.15.0 Use the goals <code>pmd:aggregate-pmd</code> and <code>pmd:aggregate-cpd</code>
* instead.
* instead. See <a href="https://maven.apache.org/plugins/maven-pmd-plugin/faq.html#typeresolution_aggregate">FAQ:
* Why do I get sometimes false positive and/or false negative violations?</a> for an explanation.
*/
@Parameter(property = "aggregate", defaultValue = "false")
@Deprecated
Expand Down
22 changes: 5 additions & 17 deletions src/main/java/org/apache/maven/plugins/pmd/PmdReport.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -513,23 +513,11 @@ private String determineAuxClasspath() throws MavenReportException {
resolvedArtifact.getArtifact().getFile().toString());
}

List<String> projectClasspath = includeTests
? localProject.getTestClasspathElements()
: localProject.getCompileClasspathElements();

// Add the project's target folder first
classpath.addAll(projectClasspath);
if (!localProject.isExecutionRoot()) {
for (String path : projectClasspath) {
File pathFile = new File(path);
String[] children = pathFile.list();

if (!pathFile.exists() || (children != null && children.length == 0)) {
getLog().warn("The project " + localProject.getArtifactId()
+ " does not seem to be compiled. PMD results might be inaccurate.");
Comment on lines -528 to -529
Copy link
Member

Choose a reason for hiding this comment

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

I added this warning back then, because users were configuring pmd plugin in a multi-module project with aggregate=true, which has the following effect: PMD is run on the parent project first, then the modules are built - which is simply the wrong order. PMD needs to be run after the project is build.

See my comment here: https://issues.apache.org/jira/browse/MPMD-277?focusedCommentId=16814718&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-16814718

If we remove the warning, we should also remove it from the FAQ:

maven-pmd-plugin/src/site/fml/faq.fml

Lines 102 to 135 in 70fcdd0

<faq id="typeresolution aggregate">
<question>
What does the warning "The project xyz does not seem to be compiled. PMD results might be inaccurate." mean?
</question>
<answer>
<p>
In order to improve PMD's results, type resolution should be used. It is enabled by default
(property <a href="pmd-mojo.html#typeResolution">typeResolution</a>) and helps to avoid false positive
findings by matching the exact types of method parameters or variables.
</p>
<p>
However, this requires that the project is built first, so that not only the project's dependencies
can be used for type resolution, but also the project's classes as well.
</p>
<p>
When using the property <a href="pmd-mojo.html#aggregate">aggregate</a>, this is problematic: With
aggregate=true, PMD is executed at the root of a multi-module project <em>before the individual
modules are built</em>. Then the types of the individual projects are not available, which might lead to
false positive findings e.g. for the rule "UnusedPrivateMethod".
If this might be the case, then the warning "The project xyz does not seem
to be compiled. PMD results might be inaccurate" is issued.
</p>
<p>
In order to use type resolution and aggregate together, maven needs to be execute in two passes:
First pass will compile the projects (e.g. <code>mvn clean package</code>) and the second pass
will execute PMD without clean via the verify phase (e.g. <code>mvn verify</code>).
</p>
<p>
Since version 3.15.0 the new goal <a href="aggregate-pmd-mojo.html">aggregate-pmd</a> can be used
which allows to run everything with only one maven call. However, this goal invokes the lifecycle
<em>test-compile</em> before executing itself, which might lead to duplicated execution of some
plugins.
</p>
</answer>

Maybe the warning should only be output, when the executed goal is pmd? The problem is, users can run "mvn clean pmd:pmd" and get false positives/negatives. Alternatively PMD could throw/report errors, if it doesn't find any classes it needs during analysis. I think, currently we don't report these.

On another note: the parameter typeResolution should probably be deprecated as PMD doesn't make sense anymore without typeresolution. There was a time, when this was a new feature and not widely used by rules, but today most rules depend on that (and the feature is always enabled in PMD but doesn't work correctly if the auxclasspath is not provided or empty).

Copy link
Member Author

Choose a reason for hiding this comment

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

Let's deprecate typeResolution first with an explanation. That is cheap to do.

Copy link
Member Author

Choose a reason for hiding this comment

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

I see. Maybe this goal should fork compile or test-compile?

Copy link
Member Author

Choose a reason for hiding this comment

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

I have the feeling that the goals need a logical cleanup with non-agggregate/aggregate compared to other plugins which need to for something...

Copy link
Member

Choose a reason for hiding this comment

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

I don't know if that's possible without breaking anything... I think it has to do, that the goal pmd is also a reporting mojo (that was probably the very first feature) as well as creating the pmd.xml file with the report, that is used by the goal verify to maybe fail the build (verify forks pmd) (and this feature was probably added on top just reusing the reporting mojo...). Not sure how other plugins deal with this (e.g. checkstyle, spotbugs, ...). What I've seen is, that m-pmd-p tries to avoid being called multiple times (which makes sense, it needs to run only once; see the canGenerateReport method which actually executes PMD, and executeReport only renders the result).

If we would add the fork to compile or test-compile, would the compile phase be called multiple times? e.g. mvn verify -> compile -> pmd:check >> pmd:pmd >> compile?

For the concrete issue MPMD-399, I would either remove the warning message (and ignore this whole discussion: if m-pmd-p is used correctly, there are no problems). Or try to improve the logic, when we issue the warning (we can easily fix the target/classes and/or target/test-classes is missing/empty - but there might be other cases. Maybe there is a different way to figure out, if the project has been compiled already).

Copy link
Member Author

@michael-o michael-o Jun 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I doubt that the goals can be made right without breaking something. We also should split report and validation goals. We should move this to a separate dicussion. Too broad for now.

If we would add the fork to compile or test-compile, would the compile phase be called multiple times? e.g. mvn verify -> compile -> pmd:check >> pmd:pmd >> compile?

Could be the case w/o an analysis.

For the concrete issue MPMD-399, I would either remove the warning message (and ignore this whole discussion: if m-pmd-p is used correctly, there are no problems). Or try to improve the logic, when we issue the warning (we can easily fix the target/classes and/or target/test-classes is missing/empty - but there might be other cases. Maybe there is a different way to figure out, if the project has been compiled already).

One the problems, as layed out in MPMD-399, is that if the param is set in the parent and not all plugins have either/proper combo there will be always a warning. For now, I would not put too much logic into it. You know PMD best. If you think that the warning should stay as a safe guard I am fine with that. I trust you, if you think that the confusion is not worth the warning, we should remove it.

Copy link
Member

Choose a reason for hiding this comment

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

Let's remove the warning. In MPMD-399 m-pmd-p was already correctly called (via the extra aggregate-pmd goal).

I've updated the FAQ entry to remove the warning message and linked from the (already deprecated) aggregate property to this FAQ entry. That should be sufficient.

}
}
}
// Add the project's classes first
classpath.addAll(
includeTests
? localProject.getTestClasspathElements()
: localProject.getCompileClasspathElements());
}

// Add the dependencies as last entries
Expand Down
13 changes: 6 additions & 7 deletions src/site/fml/faq.fml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,25 +101,24 @@ under the License.
</faq>
<faq id="typeresolution aggregate">
<question>
What does the warning "The project xyz does not seem to be compiled. PMD results might be inaccurate." mean?
Why do I get sometimes false positive and/or false negative violations?
</question>
<answer>
<p>
In order to improve PMD's results, type resolution should be used. It is enabled by default
(property <a href="pmd-mojo.html#typeResolution">typeResolution</a>) and helps to avoid false positive
findings by matching the exact types of method parameters or variables.
or false negative findings by matching the exact types of method parameters or variables.
</p>
<p>
However, this requires that the project is built first, so that not only the project's dependencies
However, this requires that the project is built first so that not only the project's dependencies
can be used for type resolution, but also the project's classes as well.
</p>
<p>
When using the property <a href="pmd-mojo.html#aggregate">aggregate</a>, this is problematic: With
aggregate=true, PMD is executed at the root of a multi-module project <em>before the individual
modules are built</em>. Then the types of the individual projects are not available, which might lead to
false positive findings e.g. for the rule "UnusedPrivateMethod".
If this might be the case, then the warning "The project xyz does not seem
to be compiled. PMD results might be inaccurate" is issued.
false positive findings e.g. for the rule "UnusedPrivateMethod". That's why this property has
been deprecated.
</p>
<p>
In order to use type resolution and aggregate together, maven needs to be execute in two passes:
Expand All @@ -135,4 +134,4 @@ under the License.
</answer>
</faq>
</part>
</faqs>
</faqs>