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

8301143: [TESTBUG] jfr/event/sampling/TestNative was backported to JDK8u without proper native wrapper #235

Closed
wants to merge 6 commits into from

Conversation

judovana
Copy link
Contributor

@judovana judovana commented Jan 24, 2023

This test was brought in by 8223147: JFR Backport and never passed. Despite not being wrong nor having jfr broken in jdk8. jdk8 testsuite simply lacks native support cases.

This is adding the shell wrapper which is compiling and properly setting up the native longSleep in native shared object.

This commit was tested only on linux.
After this commit whole jdk_jfr group passes


Progress

  • Change must be properly reviewed (1 review required, with at least 1 Reviewer)
  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue

Issue

  • JDK-8301143: [TESTBUG] jfr/event/sampling/TestNative was backported to JDK8u without proper native wrapper

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk8u-dev pull/235/head:pull/235
$ git checkout pull/235

Update a local copy of the PR:
$ git checkout pull/235
$ git pull https://git.openjdk.org/jdk8u-dev pull/235/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 235

View PR using the GUI difftool:
$ git pr show -t 235

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk8u-dev/pull/235.diff

This test was brouht in by 8223147: JFR Backport and never passed.
Despite not being wrong nor having jfr broken in jdk8.
jdk8 testsuite simply lacks native support casses.

This is adding the shell wrapper which is compiling and properly setting up
the native longSleep  in native shared object.

This comit was tested only on linux.
After this commit whole jdk_jfr group passes
Would be nice if original authors egahlin and mgronlun doublechekced if
the test is still testing  the same
@judovana
Copy link
Contributor Author

Would be nice if original authors @egahlin and @mgronlun doublechekced if the test is still testing the same

@judovana
Copy link
Contributor Author

judovana commented Jan 24, 2023

This do not have yet bugid. Will create and adapt as needed if this fix will prove worthy. Note, that the test is ok in jdk11 and up.

@bridgekeeper
Copy link

bridgekeeper bot commented Jan 24, 2023

👋 Welcome back jvanek! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@zzambers
Copy link
Contributor

Yy, shell wrapper will be necessary to make this work on JDK8. (This wrapper is inspired by [1], I think.)
I have 2 remarks:

MacOS testing

  • do we want to enable this test on MacOS? (if so, it should probably be tested there)

Linux multilib

  • on linux there is also multilib scenario, which is a bit tricky
  • basically when building native test libs for 32-bit JDK running on 64-bit system, compiler flag is needed to build 32-bit test libs
  • for hotspot tests this case is now detected in test_env.sh file (See this PR for details [2])
  • question is: should this be done in test or should there be something like test_env.sh for jdk tests as well?

[1] https://github.com/openjdk/jdk8u-dev/blob/a5bd9018d5671f2399272b680e83239154443b11/jdk/test/jdk/tools/launcher/JliLaunchTest.sh
[2] #173

@judovana
Copy link
Contributor Author

Thaxn for the ==/=. Fixed now.

The multilib chaneg sounds jsut like

source ${JDK_TOPDIR}/hotspot/test/test_env.sh
...
$gcc ...${CFLAGBITS} ...

Which sounds like safe tor. Thanx a lot!

As for MacOs, it should work. I can exclude the Darwin from enumerating. I do not have any mAcOs around.

@judovana
Copy link
Contributor Author

@judovana judovana changed the title fixed jfr/event/sampling/TestNative test 8301143 jfr/event/sampling/TestNative was backported to JDK8u without proper native wrapper Jan 26, 2023
@judovana judovana changed the title 8301143 jfr/event/sampling/TestNative was backported to JDK8u without proper native wrapper 8301143: jfr/event/sampling/TestNative was backported to JDK8u without proper native wrapper Jan 26, 2023
@openjdk openjdk bot changed the title 8301143: jfr/event/sampling/TestNative was backported to JDK8u without proper native wrapper 8301143: jfr/event/sampling/TestNative was backported to JDK8u without proper native wrapper Jan 26, 2023
@openjdk openjdk bot added the rfr Pull request is ready for review label Jan 26, 2023
@judovana judovana changed the title 8301143: jfr/event/sampling/TestNative was backported to JDK8u without proper native wrapper 8301143: [TESTBUG] jfr/event/sampling/TestNative was backported to JDK8u without proper native wrapper Jan 26, 2023
@mlbridge
Copy link

mlbridge bot commented Jan 26, 2023

Webrevs

@judovana
Copy link
Contributor Author

judovana commented Feb 13, 2023

@egahlin , @mgronlun - although not an jdk8 reviewrs, may you eyball please?
@phohensee , may I be so bold and kindly ask you for official review? (sorry, you popped out in #234)

Copy link
Contributor

@jerboaa jerboaa left a comment

Choose a reason for hiding this comment

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

Looks OK to me.

JAVA="${TESTJAVA}/bin/java"
TEST_ENV_SH="${JDK_TOPDIR}/../hotspot/test/test_env.sh"

ls -l "${TEST_ENV_SH}"
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like a debug aid. Please remove before pushing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope. It is here to fail the test if the file is missing.
See below, the +e/ source -e - the test_env.sh is not designed to run with -e.

Ok to go with this in?

Copy link
Contributor

Choose a reason for hiding this comment

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

Fair enough. Feel free to proceed.

@openjdk
Copy link

openjdk bot commented Feb 16, 2023

@judovana This change now passes all automated pre-integration checks.

After integration, the commit message for the final commit will be:

8301143: [TESTBUG] jfr/event/sampling/TestNative was backported to JDK8u without proper native wrapper

Reviewed-by: sgehwolf, phh

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 37 new commits pushed to the master branch:

  • aed31d9: 8270317: Large Allocation in CipherSuite
  • 713c020: 8301760: Fix possible leak in SpNegoContext dispose
  • 71108a5: 8287463: JFR: Disable TestDevNull.java on Windows
  • ec53e00: 8282947: JFR: Dump on shutdown live-locks in some conditions
  • c183dc5: 8142540: [TEST_BUG] Test sun/awt/dnd/8024061/bug8024061.java fails on ubuntu
  • cbde744: 8137101: [TEST_BUG] javax/swing/plaf/basic/BasicHTML/4251579/bug4251579.java failure due to timing
  • 29a1b79: 7124238: [macosx] Font in BasicHTML document is bigger than it should be
  • 3af19d3: 8282511: Use fixed certificate validation date in SSLExampleCert template
  • 8995356: 6734341: REGTEST fails: SelectionAutoscrollTest.html
  • 11a96a7: 8156581: Cleanup of ProblemList.txt
  • ... and 27 more: https://git.openjdk.org/jdk8u-dev/compare/a318e48e205acd2e15c50afb9c70c4106ed72463...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@jerboaa, @phohensee) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Feb 16, 2023
@judovana
Copy link
Contributor Author

/integrate

@openjdk openjdk bot added the sponsor Pull request is ready to be sponsored label Feb 16, 2023
@openjdk
Copy link

openjdk bot commented Feb 16, 2023

@judovana
Your change (at version eade4b4) is now ready to be sponsored by a Committer.

Copy link
Member

@phohensee phohensee left a comment

Choose a reason for hiding this comment

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

Lgtm.

@phohensee
Copy link
Member

/sponsor

@openjdk
Copy link

openjdk bot commented Feb 16, 2023

Going to push as commit b3e2380.
Since your change was applied there have been 37 commits pushed to the master branch:

  • aed31d9: 8270317: Large Allocation in CipherSuite
  • 713c020: 8301760: Fix possible leak in SpNegoContext dispose
  • 71108a5: 8287463: JFR: Disable TestDevNull.java on Windows
  • ec53e00: 8282947: JFR: Dump on shutdown live-locks in some conditions
  • c183dc5: 8142540: [TEST_BUG] Test sun/awt/dnd/8024061/bug8024061.java fails on ubuntu
  • cbde744: 8137101: [TEST_BUG] javax/swing/plaf/basic/BasicHTML/4251579/bug4251579.java failure due to timing
  • 29a1b79: 7124238: [macosx] Font in BasicHTML document is bigger than it should be
  • 3af19d3: 8282511: Use fixed certificate validation date in SSLExampleCert template
  • 8995356: 6734341: REGTEST fails: SelectionAutoscrollTest.html
  • 11a96a7: 8156581: Cleanup of ProblemList.txt
  • ... and 27 more: https://git.openjdk.org/jdk8u-dev/compare/a318e48e205acd2e15c50afb9c70c4106ed72463...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Feb 16, 2023
@openjdk openjdk bot closed this Feb 16, 2023
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Feb 16, 2023
@openjdk
Copy link

openjdk bot commented Feb 16, 2023

@phohensee @judovana Pushed as commit b3e2380.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integrated Pull request has been integrated
4 participants