validate file based arguments point to existing files / directories #483

jtnord · 2023-03-03T13:23:44Z

if a File based paramter is set (excluding workingDir which we create) then parsing the arg will fail early rather than continuing and then erroring (or silently swallowing it) later in the run

fixes #449

tested with various arg combintions with option args present and valid, not present, and present and invalid

java -jar target\plugins-compat-tester-cli.jar --working-dir=work --war=LICENSE.txt --mvn=LICENSE.txt --external-hooks-jars=LICENSE.txt,pom.xml --maven-settings=sdf
Invalid value for option '--maven-settings': Specified file sdf does not exist
Usage: pct [-hV] [--[no-]fail-fast]
...

Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
Ensure that the pull request title represents the desired changelog entry
Please describe what you did
Link to relevant issues in GitHub or Jira
Link to relevant pull requests, esp. upstream and downstream changes
Ensure you have provided tests - that demonstrates feature works or fixes the issue

if a File based paramter is set (excluding workingDir which we create) then parsing the arg will fail early rather than continuing and then erroring (or silently swallowing it) later in the run fixes #449

basil

Very nice

jtnord · 2023-03-03T13:27:49Z

src/main/java/org/jenkins/tools/test/picocli/ExistingFileTypeConverter.java

+/**
+ * Converter that converts to a File that must exist (either as a directory or a file)
+ */
+public class ExistingFileTypeConverter implements ITypeConverter<File> {


a more "rich" solution would have been to use JSR-380 based validation - this would pull in more code and still requires us to write a customer validator.

jtnord · 2023-03-03T14:58:49Z

src/main/java/org/jenkins/tools/test/PluginCompatTesterCli.java

@@ -57,7 +58,8 @@ public class PluginCompatTesterCli implements Callable<Integer> {
    @CommandLine.Option(
            names = {"-w", "--war"},
            required = true,
-            description = "Path to the WAR file to be used by the PCT.")
+            description = "Path to the WAR file to be used by the PCT.",
+            converter = ExistingFileTypeConverter.class)


whilst we could have set the default, workingDir would need to be default and we can not reference the default converters so would need yet another class.

jtnord · 2023-03-03T15:01:48Z

src/main/java/org/jenkins/tools/test/PluginCompatTesterCli.java

+    @CommandLine.Option(
+            names = "--mvn",
+            description = "The path to the Maven executable.",
+            converter = ExistingFileTypeConverter.class)


arguably this is incorrect but we already call getAbosultePath so we can not pass in something that we expect to be on the path.

jtnord · 2023-03-03T15:47:43Z

java17 build seems to be failing in the smoke test with /tmp/junit7880214900056192275/text-finder failed with exit status 143

This smells like a used to much memory and I killed you issue. given we are using multiple JVMs (maven, surefire, running the pct which runs maven and builds....) I expect we have an unconstrained JVM or 2 in the loop.

jtnord · 2023-03-03T16:18:36Z

.mvn/jvm.config

@@ -0,0 +1 @@
+-Xmx128m -XX:+HeapDumpOnOutOfMemoryError -XX:+TieredCompilation -XX:TieredStopAtLevel=1


constrain the maven jvm memory. and don't attempt to to expensive JITing for a short lived process.

no profiling has been done - tested with 64m with mvn clean install and the build ran, doubled it for good measure

jtnord · 2023-03-03T16:30:46Z

pom.xml

+              PluginCompatTesterTest runs the PCT which will run various maven.  if the JVM is not configured (its not for the text-finder)
+               then we run a JVM that seems the host/container memory and assumes it is the only thing running leading to it being OOMKilled
+               -->
+            <MAVEN_OPTS>-Xmx256m -XX:+TieredCompilation -XX:TieredStopAtLevel=1</MAVEN_OPTS>


64m caused OOMexceptions, 128 was ok, doubled that number.

jtnord · 2023-03-03T16:31:00Z

pom.xml

@@ -296,9 +297,18 @@
        <artifactId>maven-surefire-plugin</artifactId>
        <!-- Version specified in parent POM -->
        <configuration>
+          <argLine>@{argLine} -Xmx128m -XX:+HeapDumpOnOutOfMemoryError -XX:+TieredCompilation -XX:TieredStopAtLevel=1</argLine>


surefire is runnning the PCT code - no profiling has been performed.

jtnord · 2023-03-03T16:32:25Z

pom.xml

+               -->
+            <MAVEN_OPTS>-Xmx256m -XX:+TieredCompilation -XX:TieredStopAtLevel=1</MAVEN_OPTS>
+          </environmentVariables>
+          <reuseForks>false</reuseForks>


the PluginCompatTesterTest runs first which consumes the most amount of memory.
adds a little onto the build time - but for the sake of stable builds this is eaten up into the time for the smoke test.

jtnord · 2023-03-03T16:42:59Z

test run timing

	`master` (one less test)	this pr
Java 11	2m 07s	2m 34s
Java 17	3m 27s	2m 29s

jtnord added the enhancement label Mar 3, 2023

jtnord changed the title ~~simple validation for file based parameters~~ validation file based arguments point to existing files / directories Mar 3, 2023

simple validation for file based parameters

943a678

if a File based paramter is set (excluding workingDir which we create) then parsing the arg will fail early rather than continuing and then erroring (or silently swallowing it) later in the run fixes #449

jtnord force-pushed the check-file-params branch from baeaff5 to 943a678 Compare March 3, 2023 14:56

basil approved these changes Mar 3, 2023

View reviewed changes

jtnord commented Mar 3, 2023

View reviewed changes

jtnord changed the title ~~validation file based arguments point to existing files / directories~~ validate file based arguments point to existing files / directories Mar 3, 2023

Address spotbugs warning

0077ac6

jtnord force-pushed the check-file-params branch from 39c6d25 to ffe0035 Compare March 3, 2023 16:10

try and constrain the JVM to avoid OOMs

7be7b5e

jtnord force-pushed the check-file-params branch from ffe0035 to 7be7b5e Compare March 3, 2023 16:30

jtnord commented Mar 3, 2023

View reviewed changes

jtnord requested a review from basil March 3, 2023 16:43

jtnord marked this pull request as ready for review March 3, 2023 16:43

jtnord requested a review from a team as a code owner March 3, 2023 16:43

basil approved these changes Mar 3, 2023

View reviewed changes

jtnord merged commit e2258a1 into master Mar 3, 2023

jtnord deleted the check-file-params branch March 3, 2023 17:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

validate file based arguments point to existing files / directories #483

validate file based arguments point to existing files / directories #483

jtnord commented Mar 3, 2023 •

edited

Loading

basil left a comment

jtnord Mar 3, 2023

jtnord Mar 3, 2023

jtnord Mar 3, 2023

jtnord commented Mar 3, 2023

jtnord Mar 3, 2023

jtnord Mar 3, 2023

jtnord Mar 3, 2023

jtnord Mar 3, 2023

jtnord commented Mar 3, 2023

		@@ -0,0 +1 @@
		-Xmx128m -XX:+HeapDumpOnOutOfMemoryError -XX:+TieredCompilation -XX:TieredStopAtLevel=1

validate file based arguments point to existing files / directories #483

validate file based arguments point to existing files / directories #483

Conversation

jtnord commented Mar 3, 2023 • edited Loading

basil left a comment

Choose a reason for hiding this comment

jtnord Mar 3, 2023

Choose a reason for hiding this comment

jtnord Mar 3, 2023

Choose a reason for hiding this comment

jtnord Mar 3, 2023

Choose a reason for hiding this comment

jtnord commented Mar 3, 2023

jtnord Mar 3, 2023

Choose a reason for hiding this comment

jtnord Mar 3, 2023

Choose a reason for hiding this comment

jtnord Mar 3, 2023

Choose a reason for hiding this comment

jtnord Mar 3, 2023

Choose a reason for hiding this comment

jtnord commented Mar 3, 2023

jtnord commented Mar 3, 2023 •

edited

Loading