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

Fixing builds on Windows #24606

Merged
merged 11 commits into from
Oct 3, 2023
Merged

Fixing builds on Windows #24606

merged 11 commits into from
Oct 3, 2023

Conversation

dmatej
Copy link
Contributor

@dmatej dmatej commented Sep 25, 2023

I would not recommend MS Windows for a development to anyone, however ... I would like to see the build passing at least basically. Also tests are useful to see differences between operating systems and can suggest some solutions of possible issues.

@dmatej dmatej self-assigned this Sep 25, 2023
@dmatej dmatej added bug Something isn't working build and test improvement labels Sep 25, 2023
@avpinchuk
Copy link
Contributor

To save your time, part 1:

assertThat(cmdline, hasItems(endsWith("java"), is("-cp"), is("-XX:+UnlockDiagnosticVMOptions"), is("-verbose")));

hasItems(endsWith("java"), is("-cp"), not(is("-XX:+UnlockDiagnosticVMOptions")), is("-verbose")));

endsWith("java") works on UNIX but failed on Windows due to java.exe.

Also we should disable with @DisabledOnOs(OS.WINDOWS) for whole test class:

Asterisk * improperly quoted on Windows and asadmin get * command fails:

@dmatej
Copy link
Contributor Author

dmatej commented Sep 25, 2023

To save your time, part 1:

assertThat(cmdline, hasItems(endsWith("java"), is("-cp"), is("-XX:+UnlockDiagnosticVMOptions"), is("-verbose")));

hasItems(endsWith("java"), is("-cp"), not(is("-XX:+UnlockDiagnosticVMOptions")), is("-verbose")));

endsWith("java") works on UNIX but failed on Windows due to java.exe.

Also we should disable with @DisabledOnOs(OS.WINDOWS) for whole test class:

Asterisk * improperly quoted on Windows and asadmin get * command fails:

Bit late, I already did that, but at least we have same ideas :-)
I ended with this exception in server.log, which is probably caused by some security software not under my control. So I will just push what I have for now ...

[2023-09-25T21:52:29.061483+02:00] [GF 7.0.9-SNAPSHOT] [WARNING] [NCLS-JMX-00007] [jakarta.enterprise.system.jmx] [tid: _ThreadID=82 _ThreadName=Thread-9] [levelValue: 900] [[
  Cannot start JMX connector JmxConnector config: { name = system, Protocol = rmi_jrmp, Address = 0.0.0.0, Port = 8686, AcceptAll = false, AuthRealmName = admin-realm, SecurityEnabled = false} due to exception java.io.IOException: Cannot bind to URL [rmi://xxxxx:8686/jmxrmi]: javax.naming.ServiceUnavailableException [Root exception is java.rmi.ConnectException: Connection refused to host:xxxxx; nested exception is: 
	java.net.ConnectException: Connection timed out: connect]]]

I cannot make the host name public, but it is from some managed windows domain and I have no rights to expose port on this machine.

@@ -36,6 +36,6 @@ class StartStopITest {
*/
@Test
void asadminGet() {
assertThat(ASADMIN.exec("get", "*"), asadminOK());
assertThat(ASADMIN.exec("get", "'*'"), asadminOK());
Copy link
Contributor

Choose a reason for hiding this comment

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

On Windows work with double quotes, on Linux - no. We need some condition here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Aha, I thought it passed on my Windows, but this test failed to start the domain (unable to expose ports). Can you finish the PR? I can't do more on this machine. I just used the opportunity to try it :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure ;)

@arjantijms arjantijms modified the milestones: 7.0.9, 7.0.10 Sep 26, 2023
@avpinchuk avpinchuk marked this pull request as draft September 26, 2023 21:24
@avpinchuk
Copy link
Contributor

This PR #24610 should fix OSGICommandsITest on Windows.

@dmatej dmatej marked this pull request as ready for review September 28, 2023 21:57
@dmatej dmatej marked this pull request as draft September 28, 2023 21:58
@dmatej
Copy link
Contributor Author

dmatej commented Sep 28, 2023

Aha, I did not notice you switched it back to Draft, @avpinchuk

@dmatej dmatej assigned avpinchuk and unassigned dmatej Sep 28, 2023
@avpinchuk
Copy link
Contributor

Yes. This is because we need to close several bugs in GF, not only in tests ;)

@dmatej
Copy link
Contributor Author

dmatej commented Sep 29, 2023

Yes. This is because we need to close several bugs in GF, not only in tests ;)

Ok, I will leave it on your decision for now, thank you.

@avpinchuk
Copy link
Contributor

Patch #24611 fixes resource disposition on Windows platform. Now it works:

@avpinchuk
Copy link
Contributor

On Windows, Files.copy(src, dst, attrs) set actual creationTime and lastAcccessTime for dst, but copy lastModifiedTime from src.

Thus, JspReloadGeneratedServletIfUpdatedTest failed because JspReloadGeneratedServletIfUpdated_jsp compiled at build time and has earlier lastModifiedTime than generated by JSP engine. When we copying build-time version instead of generated, its modified timestamp left unchanged.

@avpinchuk
Copy link
Contributor

Windows 10 Pro 22H2 (19045.3448):

mvn clean install -Pstaging

image

@avpinchuk
Copy link
Contributor

@dmatej, all done.

dmatej and others added 10 commits October 2, 2023 21:29
Signed-off-by: David Matějček <david.matejcek@omnifish.ee>
Signed-off-by: David Matějček <david.matejcek@omnifish.ee>
Signed-off-by: David Matějček <david.matejcek@omnifish.ee>
Signed-off-by: David Matějček <david.matejcek@omnifish.ee>
- Unix feature

Signed-off-by: David Matějček <david.matejcek@omnifish.ee>
Signed-off-by: David Matějček <david.matejcek@omnifish.ee>
Signed-off-by: David Matějček <david.matejcek@omnifish.ee>
Signed-off-by: David Matějček <david.matejcek@omnifish.ee>
Signed-off-by: Alexander Pinčuk <alexander.v.pinchuk@gmail.com>
Signed-off-by: Alexander Pinčuk <alexander.v.pinchuk@gmail.com>
@dmatej
Copy link
Contributor Author

dmatej commented Oct 2, 2023

I have yet one failure, the exec plugin execution (and same previous gfbuild-maven-plugin:exec) were depending on OS java command, same as #24617 , I will fix it now.

- if it executed asadmin, it used OS default java

Signed-off-by: David Matějček <david.matejcek@omnifish.ee>
@avpinchuk
Copy link
Contributor

I opened yet another PR to WaSP eclipse-ee4j/wasp#61

@avpinchuk
Copy link
Contributor

avpinchuk commented Oct 2, 2023

I started the build... BUILD SUCCESS

@dmatej
Copy link
Contributor Author

dmatej commented Oct 3, 2023

I started the build... BUILD SUCCESS

So, let's make it Ready for review, do you agree?

@avpinchuk
Copy link
Contributor

So, let's make it Ready for review, do you agree?

Yes, sure.

@dmatej dmatej marked this pull request as ready for review October 3, 2023 13:57
@arjantijms
Copy link
Contributor

LGTM, although I didn't test on Windows. The WaSP PR has some good comments btw ;)

@dmatej dmatej merged commit 6c70a06 into eclipse-ee4j:master Oct 3, 2023
2 checks passed
@avpinchuk
Copy link
Contributor

The WaSP PR has some good comments btw ;)

I pushed a fixed version

@dmatej dmatej deleted the windows branch October 3, 2023 23:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working build and test improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants