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

Enable Windows builds #4447

Closed
wants to merge 10 commits into from
Closed

Enable Windows builds #4447

wants to merge 10 commits into from

Conversation

jglick
Copy link
Member

@jglick jglick commented Jan 22, 2020

See #4178 (comment) for context.

@jglick jglick requested review from slide and oleg-nenashev January 22, 2020 17:37
Copy link
Member

@slide slide left a comment

Choose a reason for hiding this comment

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

I am looking at when the 11.0.6 release of the base image will be available, then we can hopefully restart doing the jdk11 builds, but this looks good to me for what we can currently do.

@jglick
Copy link
Member Author

jglick commented Jan 22, 2020

restart doing the jdk11 builds

As in #4178, I do not actually care that much about this—testing Linux/8 + Windows/8 + Linux/11 is probably adequate coverage; the main problem I was hitting seems to be a lack of memory for the Windows/8 builds, which I cannot work around at this level. (If we switched our build environment to Kubernetes then I could use a podTemplate requesting a specific memory size.)

@jglick
Copy link
Member Author

jglick commented Jan 22, 2020

Getting the same cleanup errors in DisablePluginCommandTest, suggesting that at least some of the problems are genuine regressions (at least in tests) introduced since Windows CI was disabled.

@jglick
Copy link
Member Author

jglick commented Jan 22, 2020

ResourceDomainTest failures are real, as perhaps are the Security914Test failures.

The others are problems in TemporaryDirectoryAllocator.dispose which I can reproduce in a Windows 10 VM.

@jglick
Copy link
Member Author

jglick commented Jan 22, 2020

@jglick
Copy link
Member Author

jglick commented Jan 22, 2020

@slonopotamus @jtnord FYI

@jglick
Copy link
Member Author

jglick commented Jan 22, 2020

#4382 is also related

Jenkinsfile Outdated
sh 'git add . && git diff --exit-code HEAD'
} else {
bat mvnCmd
// TODO tool 'mvn' is still an old version without -ntp
Copy link
Member

Choose a reason for hiding this comment

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

eek... maybe ToolInstallations would help here (anyway still waiting for INFRA-2375 / INFRA-2080 )

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah just waiting for 2080.

<id>all-tests</id>
<activation>
<property>
<name>!test</name>
Copy link
Member

Choose a reason for hiding this comment

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

should be true even when using -Dtest (because multiple forks and you can specify multiple tests)

Copy link
Member Author

Choose a reason for hiding this comment

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

Just copying from plugin-pom. Skipped when -Dtest=SomeTest is specified in case this is a developer run.

Copy link
Member

Choose a reason for hiding this comment

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

I possibly should have cleaned that up when I was there.
problem is if you use -Dtest=SomeTest,OtherTest -DforkCOunt=2 your output is garbage

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it is hard to read. Still better IMO than showing nothing.

@@ -25,6 +28,8 @@
*/
@Test
public void runSampleBenchmark() throws Exception {
assumeFalse(Functions.isWindows());
Copy link
Member

Choose a reason for hiding this comment

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

benchmarks don't work on windows?

Copy link
Member Author

Choose a reason for hiding this comment

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

Dunno, had some problems here and it did not seem important.

@jtnord
Copy link
Member

jtnord commented Jan 22, 2020

well a quick look at PluginManager I found one bug (but not the one you are looking for...)

if (!filter.accept(new File(url.getFile()).getParentFile(), fileName)) {

@jglick
Copy link
Member Author

jglick commented Jan 23, 2020

mvn -pl test surefire:test -Dsurefire.skipAfterFailureCount=1 -Dtest=DisablePluginCommandTest

fails even if I delete the bodies of all the test methods. Thus I think that it is @WithPlugin which is to blame, not disabling plugins.

@jglick
Copy link
Member Author

jglick commented Jan 23, 2020

I tried this, without success (no plugin JARs show up in the list of open file handles, despite the error referring to a handle on matrix-project.jar):

diff --git test/pom.xml test/pom.xml
index b955406376..bab03c95b0 100644
--- test/pom.xml
+++ test/pom.xml
@@ -227,6 +227,23 @@ THE SOFTWARE.
       <artifactId>objenesis</artifactId>
       <scope>test</scope>
     </dependency>
+    <dependency>
+      <groupId>org.kohsuke</groupId>
+      <artifactId>file-leak-detector</artifactId>
+      <version>1.13</version>
+      <classifier>jar-with-dependencies</classifier>
+      <scope>test</scope>
+      <exclusions>
+        <exclusion>
+          <groupId>args4j</groupId>
+          <artifactId>args4j</artifactId>
+        </exclusion>
+        <exclusion>
+          <groupId>org.kohsuke</groupId>
+          <artifactId>asm6</artifactId>
+        </exclusion>
+      </exclusions>
+    </dependency>
   </dependencies>
 
   <build>
@@ -265,6 +282,12 @@ THE SOFTWARE.
               </artifactItems>
             </configuration>
           </execution>
+          <execution>
+            <id>properties</id>
+            <goals>
+              <goal>properties</goal>
+            </goals>
+          </execution>
         </executions>
       </plugin>
       <plugin>
@@ -272,7 +295,7 @@ THE SOFTWARE.
         <artifactId>maven-surefire-plugin</artifactId>
         <!-- version specified in grandparent pom -->
         <configuration>
-          <argLine>${jacocoSurefireArgs} -Dfile.encoding=UTF-8 -Xmx1g -Djdk.net.URLClassPath.disableClassPathURLCheck=true</argLine>
+          <argLine>${jacocoSurefireArgs} -Dfile.encoding=UTF-8 -Xmx1g -Djdk.net.URLClassPath.disableClassPathURLCheck=true -javaagent:${org.kohsuke:file-leak-detector:jar:jar-with-dependencies}</argLine>
           <systemPropertyVariables>
               <!-- use AntClassLoader that supports predictable file handle release -->
               <hudson.ClassicPluginStrategy.useAntClassLoader>true</hudson.ClassicPluginStrategy.useAntClassLoader>
diff --git test/src/test/java/hudson/cli/DisablePluginCommandTest.java test/src/test/java/hudson/cli/DisablePluginCommandTest.java
index b7281ba71d..594fb8d926 100644
--- test/src/test/java/hudson/cli/DisablePluginCommandTest.java
+++ test/src/test/java/hudson/cli/DisablePluginCommandTest.java
@@ -42,15 +42,41 @@ import static hudson.cli.CLICommandInvoker.Matcher.failedWith;
 import static hudson.cli.CLICommandInvoker.Matcher.succeeded;
 import static hudson.cli.DisablePluginCommand.RETURN_CODE_NOT_DISABLED_DEPENDANTS;
 import static hudson.cli.DisablePluginCommand.RETURN_CODE_NO_SUCH_PLUGIN;
+import java.io.File;
+import java.io.PrintWriter;
+import java.util.List;
 import static org.hamcrest.Matchers.is;
 import static org.hamcrest.Matchers.notNullValue;
+import org.junit.After;
 import static org.junit.Assert.*;
 import org.junit.Ignore;
+import org.kohsuke.file_leak_detector.Listener;
 
 public class DisablePluginCommandTest {
 
     @Rule
-    public JenkinsRule j = new JenkinsRule();
+    public JenkinsRule j = new JenkinsRule() {
+        @Override
+        public void after() throws Exception {
+            try {
+                super.after();
+            } finally {
+                PrintWriter pw = new PrintWriter(System.err, true);
+                for (Listener.Record r : Listener.getCurrentOpenFiles()) {
+                    if (r instanceof Listener.FileRecord) {
+                        File f = ((Listener.FileRecord) r).file;
+                        if (f.getAbsolutePath().matches(".*plugins.*[.]jar")) {
+                            r.dump("", pw);
+                        } else {
+                            pw.println("(also " + f + ")");
+                        }
+                    } else {
+                        pw.println("(also " + r + ")");
+                    }
+                }
+            }
+        }
+    };
 
     /**
      * Can disable a plugin with an optional dependent plugin.

@jglick
Copy link
Member Author

jglick commented Jan 23, 2020

I ran handle after a failure, while the JVM remained open, but it showed no processes holding a handle onto any files in the temporary %JENKINS_HOME%. Either it cannot see that file, for whatever reason, or garbage collection kicked in and closed the handle.

@jglick
Copy link
Member Author

jglick commented Jan 23, 2020

PluginManagerTest.uploadJpi also fails, without @WithPlugin, so it is something about loading plugins dynamically rather than from the classpath I suppose.

@jglick
Copy link
Member Author

jglick commented Jan 23, 2020

Note that PluginManager.stop does call AntClassLoader.cleanup and thus JarFile.close; and JTH on Windows already disables JAR caches used for JarURLConnection.

@slonopotamus
Copy link
Contributor

slonopotamus commented Jan 23, 2020

WRT ResourceDomainTest: I believe this needs to we wrapped in try-with-resources.

Also, " 100% evil dir name " is not a valid directory name on Windows because it has trailing whitespace, that's why ResourceDomainTest.testMoreUrlEncoding fails.

@slonopotamus
Copy link
Contributor

slonopotamus commented Jan 23, 2020

QueueTest.persistenceBlockedItem - make sure build is finished before returning from test. Shutdown procedure in Jenkins rule does not properly cleanup running builds (I consider this to be a bug, actually), as was discovered in jenkinsci/workflow-cps-plugin#345. Same with RunRangeCommand2Test.dummyRangeShouldSuccessEvenTheBuildIsRunning, LazyBuildMixInTest.newRunningBuildRelationFromPrevious and several others. This is easy to diagnose, they keep build log open.

@jglick
Copy link
Member Author

jglick commented Jan 23, 2020

make sure build is finished before returning from test

Yes, I figured this class of failure out with b0e05f5, so it should be straightforward to fix the others.

" 100% evil dir name " is not a valid directory name on Windows because it has trailing whitespace

OK, can just be skipped or modified on Windows I suppose.

I believe

needs to we wrapped in try-with-resources

Sounds right.

To be clear, this miscellaneous ones I can probably fix quickly enough, though of course a PR to jglick/jenkins#windows with fixes would be appreciated. What I am stuck on is the plugin JAR locks, of which there are several examples but so far PluginManagerTest.uploadJpi seems to be the easiest to reproduce quickly.

@slonopotamus
Copy link
Contributor

I'll dig into a problem with @WithPlugin a bit later today/tomorrow.

@slide
Copy link
Member

slide commented Jan 23, 2020

FYI, the docker image for jdk11 has been updated and should now work for ACI. I have not had a chance to test it yet, but the JDK was updated to 11.0.6 which should have the fixes.

@jtnord
Copy link
Member

jtnord commented Jan 30, 2020

WRT ResourceDomainTest: I believe this needs to we wrapped in try-with-resources.

Also, " 100% evil dir name " is not a valid directory name on Windows because it has trailing whitespace, that's why ResourceDomainTest.testMoreUrlEncoding fails.

What makes you say that? it i perfectly valid (you can not create it in explorer as it tries to be helpful however...)

image

@slonopotamus
Copy link
Contributor

What makes you say that?

If you look at build log of ResourceDomainTest.testMoreUrlEncoding (https://ci.jenkins.io/blue/organizations/jenkins/Core%2Fjenkins/detail/PR-4447/10/tests/), you'll see:

java.nio.file.InvalidPathException: Trailing char < > at index 126: C:\Jenkins\workspace\Core_jenkins_PR-4447\test\target\j h6429179792960372070\root.tmp4891911103708016485\ 100% evil dir name   
	at sun.nio.fs.WindowsPathParser.normalize(WindowsPathParser.java:191)
	at sun.nio.fs.WindowsPathParser.parse(WindowsPathParser.java:153)
	at sun.nio.fs.WindowsPathParser.parse(WindowsPathParser.java:77)
	at sun.nio.fs.WindowsPath.parse(WindowsPath.java:94)
	at sun.nio.fs.WindowsFileSystem.getPath(WindowsFileSystem.java:255)
	at java.io.File.toPath(File.java:2234)
	at hudson.Util.fileToPath(Util.java:1528)
Caused: java.io.IOException
	at hudson.Util.fileToPath(Util.java:1530)
	at hudson.FilePath.mkdirs(FilePath.java:3256)
	at hudson.FilePath.access$1300(FilePath.java:211)
	at hudson.FilePath$Write.invoke(FilePath.java:2098)
	at hudson.FilePath$Write.invoke(FilePath.java:2088)
	at hudson.FilePath.act(FilePath.java:1075)
	at hudson.FilePath.act(FilePath.java:1058)
	at hudson.FilePath.write(FilePath.java:2086)
	at jenkins.security.ResourceDomainTest$RootActionImpl.doDynamic(ResourceDomainTest.java:388)
	at java.lang.invoke.MethodHandle.invokeWithArguments(MethodHandle.java:627)
	at org.kohsuke.stapler.Function$MethodFunction.invoke(Function.java:396)

@jtnord
Copy link
Member

jtnord commented Jan 30, 2020

JDK bug. https://bugs.openjdk.java.net/browse/JDK-8190546 the name is actually valid on windows, so its just that the JDK is brain dead here.

@slonopotamus
Copy link
Contributor

Okay, but they do not want to fix it. So the path of least resistance is just to accept it and use a different directory name. Especially given the fact that test doesn't really care about that whitespace.

@jglick
Copy link
Member Author

jglick commented Feb 20, 2020

What I am stuck on is the plugin JAR locks, of which there are several examples but so far PluginManagerTest.uploadJpi seems to be the easiest to reproduce quickly.

I have not found the time to get back to this, and no one else seems to have either, so closing this for now so we do not waste build resources on something known to be broken. Great task for volunteers to pick up!

@jglick jglick closed this Feb 20, 2020
@jglick jglick mentioned this pull request Dec 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants