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.

Sign up for GitHub

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

Add -Xverify:none to docs for OpenJ9 #50

Merged
merged 4 commits into from
May 30, 2023
Merged

Add -Xverify:none to docs for OpenJ9 #50

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
86c20ce
Add -Xverify:none to tests and docs
Apr 18, 2020
9c947a5
move -Xverify:none docs to OpenJ9 caveat
mandragorn May 30, 2023
686d17e
remove changes to pom.xml
mandragorn May 30, 2023
a8bb473
tweak docs
mandragorn May 30, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions README.adoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,10 @@ This project uses the JVM's support for instrumenting Java classes during startu
It adds code to various places where files or sockets are opened and closed
to print out which file descriptors have not been closed correctly.

== OpenJ9 Caveats

Since this project modifies core java bytecode it requires the `-Xverify:none` argument when running the agent when using OpenJ9. See https://github.com/jenkinsci/lib-file-leak-detector/issues/37[#37] and https://github.com/jenkinsci/lib-file-leak-detector/pull/50#issue-602359846[#50] for details.

== Documentation

* https://javadoc.jenkins.io/component/file-leak-detector/[Javadoc]
Expand Down
2 changes: 1 addition & 1 deletion pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@
<exclude>**/TransformerTest.java</exclude>
<exclude>**/AgentMainTest.java</exclude>
</excludes>
<argLine>-javaagent:"${project.build.directory}/file-leak-detector-${project.version}-jar-with-dependencies.jar"</argLine>
<argLine>-javaagent:"${project.build.directory}/file-leak-detector-${project.version}-jar-with-dependencies.jar" -Xverify:none</argLine>
Copy link
Member

Choose a reason for hiding this comment

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

This affects our test suite on HotSpot by decreasing verification. I think this PR should not affect HotSpot. The fact that this was working in our tests on HotSpot without -Xverify:none is evidence that this is only needed for OpenJ9.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

alright, sounds like this pr isn't needed. abandoning.

Copy link
Member

Choose a reason for hiding this comment

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

The change to decrease verification on HotSpot in our test suite isn't needed (as far as I can tell) and isn't wanted (by me). The change to add additional documentation for OpenJ9 users seems valid to me though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I honestly don't remember a lot of details around this from 3 years ago. I think the issue I had was I couldn't run the tests locally because I had java9 setup, I had no idea it was a hotspot issue vs j9. I thought it was a java 8 vs java 9+ issue. I assume the test suite is running in something > java 8 at this point so my initial assumptions are wrong. I don't currently have the bandwidth to test this out in different jvms to see what works and what doesn't so I'm not sure how much value the documentation actually adds w/o those details. I guess I could make the docs more vague to cover it.

Copy link
Member

Choose a reason for hiding this comment

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

We run the test suite with HotSpot (Eclipse Temurin) on Java 11 and 17. Yes, I think a general suggestion in the documentation for OpenJ9 users to run without verification should cover it.

</configuration>
</execution>
<execution>
Expand Down