-
Notifications
You must be signed in to change notification settings - Fork 78
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
Switch to spotbugs and fix some issues #69
Conversation
res0nance
commented
Aug 20, 2019
- Swapped to spotbugs and fixed multiple issues.
- There still remains some issues but the PR was getting large
Need to review it carefully, but thanks a lot! |
sorry, still in my queue |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some API changes and commented code are pretty concerning here
* @author mailto: <a href="[email protected]">Rick Knowles</a> | ||
* @version $Id: Ajp13ConnectorFactory.java,v 1.12 2006/03/24 17:24:22 rickknowles Exp $ | ||
*/ | ||
public class Ajp13ConnectorFactory implements ConnectorFactory { | ||
public boolean start(Map args, Server server) throws IOException { | ||
int listenPort = Option.AJP13_PORT.get(args); | ||
String listenAddress = Option.AJP13_LISTEN_ADDRESS.get(args); | ||
// String listenAddress = Option.AJP13_LISTEN_ADDRESS.get(args); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If unused, please.do one of the following..
- delete it
- Create a follow-up bug, link it from a TODO comment
Looks like it is a bug unless I miss something
continue; | ||
} | ||
File parentOutFile = outFile.getParentFile(); | ||
if (!parentOutFile.mkdirs()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can drop Java 6 later and start using Files
here.
CC @olamy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll clean this bit up then
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one minor compat issue, should be fine
public static OInt CONTROL_PORT = Option.integer("controlPort"); | ||
public static OInt PORT = Option.integer("port"); | ||
public static OInt DEBUG = new OInt("debug", 5) { | ||
public final static OInt CONTROL_PORT = Option.integer("controlPort"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIRC breaking change was here. Not that important
new String[][] {{"/", ""}, {"\\", ""}, {",", ""}}) + "/" : "") + | ||
"winstone/" + warfile.getName()); | ||
tempFile.delete(); | ||
if (!tempFile.delete()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or there is a Commons IO utility, or you can use java.nio.file.Files.delete
which throws a meaningful exception.
@@ -250,43 +254,52 @@ protected File getWebRoot(File requestedWebroot, File warfile) throws IOExceptio | |||
if(!timestampFile.exists() || Math.abs(timestampFile.lastModified()- warfile.lastModified())>1000) { | |||
// contents of the target directory is inconsistent from the war. | |||
deleteRecursive(unzippedDir); | |||
unzippedDir.mkdirs(); | |||
if (!unzippedDir.mkdirs()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
continue; | ||
} | ||
try { | ||
Files.createDirectories(Paths.get(outFile.getParent())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW in Jenkins core we have been catching InvalidPathException
as well, which apparently can happen under some obscure conditions on Windows servers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would recommend to address it before merging
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving the new version
…no /embedded.war).