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

Test project compilation fixes for Eclipse #1317

Merged
merged 4 commits into from
Oct 6, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,9 @@ Currently the versioning policy of this project follows [Semantic Versioning v2.

## Unreleased - 2020-??-??
### Changed
* Bump jaxen from 1.1.6 to 1.2.0 supporting Java 11 compilation ([#1316](https://github.com/spotbugs/spotbugs/issues/1316))
* Fixed error dialog on cancelling SpotBugs job in Eclipse ([#1314](https://github.com/spotbugs/spotbugs/issues/1314))
* Fixed IllegalArgumentException in OpcodeStack.constantToInt ([#893](https://github.com/spotbugs/spotbugs/issues/893))
* Bump ASM from 8.0.1 to 9.0 supporting JDK16 (sealed classes)

## 4.1.3 - 2020-09-25
Expand Down
34 changes: 27 additions & 7 deletions eclipsePlugin/src/de/tobject/findbugs/builder/FindBugsWorker.java
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
import java.util.Map.Entry;
import java.util.Set;
import java.util.TreeMap;
import java.util.concurrent.ExecutionException;
import java.util.stream.Collectors;

import org.dom4j.DocumentException;
Expand Down Expand Up @@ -303,27 +304,46 @@ private void collectClassFiles(List<WorkItem> resources, Map<IPath, IPath> outLo
* fb engine, which will be <b>disposed</b> after the analysis is
* done
*/
private void runFindBugs(final FindBugs2 findBugs) {
private static void runFindBugs(final FindBugs2 findBugs) {
if (DEBUG) {
FindbugsPlugin.log("Running findbugs in thread " + Thread.currentThread().getName());
}
System.setProperty("findbugs.progress", "true");
try {
// Perform the analysis! (note: This is not thread-safe)
findBugs.execute();
} catch (InterruptedException e) {
if (DEBUG) {
FindbugsPlugin.getDefault().logException(e, "Worker interrupted");
} catch (Exception e) {
if (isInterrupted(e)) {
if (DEBUG) {
FindbugsPlugin.getDefault().logException(e, "Worker interrupted");
}
} else {
FindbugsPlugin.getDefault().logException(e, "Error performing SpotBugs analysis");
}
Thread.currentThread().interrupt();
} catch (IOException e) {
FindbugsPlugin.getDefault().logException(e, "Error performing SpotBugs analysis");
} finally {
findBugs.dispose();
}

}

private static boolean isInterrupted(Exception e) {
if (e instanceof InterruptedException) {
Thread.currentThread().interrupt();
return true;
}
Throwable cause = e.getCause();
if (cause instanceof InterruptedException) {
return true;
}
if (cause instanceof ExecutionException) {
ExecutionException ee = (ExecutionException) cause;
if (ee.getCause() instanceof InterruptedException) {
return true;
}
}
return false;
}

/**
* Update the BugCollection for the project.
*
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
package edu.umd.cs.findbugs.ba;

import static edu.umd.cs.findbugs.test.CountMatcher.containsExactly;
import static org.hamcrest.MatcherAssert.assertThat;

import org.junit.Test;

import edu.umd.cs.findbugs.AbstractIntegrationTest;
import edu.umd.cs.findbugs.test.matcher.BugInstanceMatcher;
import edu.umd.cs.findbugs.test.matcher.BugInstanceMatcherBuilder;

public class Issue893Test extends AbstractIntegrationTest {

@Test
public void test() {
performAnalysis("ghIssues/Issue893.class");
BugInstanceMatcher bugTypeMatcher = new BugInstanceMatcherBuilder().build();
assertThat(getBugCollection(), containsExactly(0, bugTypeMatcher));
}

}
6 changes: 3 additions & 3 deletions spotbugs/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ dependencies {
exclude group: 'pull-parser', module: 'pull-parser'
exclude group: 'xpp3', module: 'xpp3'
}
implementation 'jaxen:jaxen:1.1.6' // only transitive through dom4j:dom4j:1.6.1, which has an *optional* dependency on jaxen:jaxen.
implementation 'jaxen:jaxen:1.2.0' // only transitive through dom4j:dom4j:1.6.1, which has an *optional* dependency on jaxen:jaxen.
api 'org.apache.commons:commons-lang3:3.11'
api 'org.apache.commons:commons-text:1.9'
api 'org.slf4j:slf4j-api:1.8.0-beta4'
Expand All @@ -104,7 +104,7 @@ dependencies {
guiTestImplementation sourceSets.gui.runtimeClasspath
guiTestImplementation 'junit:junit:4.13'
guiTestCompileOnly project(':spotbugs-annotations')

// for Eclipse compilation only
appleExtensions 'com.apple:AppleJavaExtensions:1.4'
}
Expand Down Expand Up @@ -361,4 +361,4 @@ spotbugsTest {
// TODO : validatesamplexmloutput

// XML validation
// TODO : validate
// TODO : validate
4 changes: 4 additions & 0 deletions spotbugs/src/main/java/edu/umd/cs/findbugs/OpcodeStack.java
Original file line number Diff line number Diff line change
Expand Up @@ -2194,6 +2194,10 @@ private int constantToInt(Item it) {
if (constant instanceof Character) {
return ((Character) constant).charValue();
}
if (constant instanceof Boolean) {
Boolean val = (Boolean) constant;
return val.booleanValue() ? 1 : 0;
}
throw new IllegalArgumentException(String.valueOf(constant));
}

Expand Down
10 changes: 10 additions & 0 deletions spotbugsTestCases/src/java/ghIssues/Issue893.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
package ghIssues;

public class Issue893 {
public boolean test() throws Exception {
Boolean b = Boolean.valueOf(true);
// Line below should not throw IllegalArgumentException during analysis
b &= Boolean.valueOf(false);
return b;
}
}
4 changes: 3 additions & 1 deletion test-harness-core/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,9 @@ apply from: "$rootDir/gradle/javadoc.gradle"
apply from: "$rootDir/gradle/maven.gradle"

dependencies {
compileOnly project(':spotbugs')
api (project(':spotbugs')) {
transitive = true
}
}

java {
Expand Down
2 changes: 1 addition & 1 deletion test-harness/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ apply from: "$rootDir/gradle/maven.gradle"

dependencies {
compileOnly 'junit:junit:4.13'
compileOnly project(':spotbugs')
testImplementation project(':spotbugs')
Copy link
Member

Choose a reason for hiding this comment

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

Could you consider to add explanations in CHANGELOG for this change?
I think users of test-harness and test-harness-jupiter needs to this change (from compile-only scope to compile scope).

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'm not sure how the changed compilation setting here can affect users. Dependencies itself aren't changed. That should be fully transparent for clients.

Copy link
Member Author

Choose a reason for hiding this comment

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

@KengoTODA : anything else is missing?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ping...

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @KengoTODA ,

to work on #1254, I had to cherry-pick the changes here. Can the changes get merged so I can work with actual master?

Best regards and thanks,
Simeon

Copy link
Member

Choose a reason for hiding this comment

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

@iloveeclipse
I think changes in test-harness affect its dependency.
That dependency will be changed.
api configuration makes test-harness dependent on that.

Copy link
Member Author

Choose a reason for hiding this comment

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

@wreulicke : not sure what do you mean by that. How this change can affect clients of test-harness? I'm not the gradle user, I have no idea why there are multiple dependencies chains invented by gradle, and how that, once a module is compiled, can affect its clients - I really can't understand that. And btw, let assume test-harness clients would be now also depend on spotbugs - what's wrong by that? How that would affect them, they had spotbugs on classpath already anyway?

My problem is that current state on master is not usable with Eclipse, so contributing is a pain, nothing compiles. That is way more severe issue as a theoretical possibility to somehlw break one or two test-harness clients that may exists in the world.

api project(':test-harness-core')
}

Expand Down