-
Notifications
You must be signed in to change notification settings - Fork 235
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
8238268: Many SA tests are not running on OSX because they do not attempt to use sudo when available #1269
8238268: Many SA tests are not running on OSX because they do not attempt to use sudo when available #1269
Conversation
👋 Welcome back clanger! A progress list of the required criteria for merging this PR into |
This backport pull request has now been updated with issue from the original commit. |
Webrevs
|
854de39
to
990171d
Compare
@RealCLanger Please do not rebase or force-push to an active PR as it invalidates existing review comments. All changes will be squashed into a single commit automatically when integrating. See OpenJDK Developers’ Guide for more information. |
990171d
to
53f80c4
Compare
@RealCLanger Please do not rebase or force-push to an active PR as it invalidates existing review comments. All changes will be squashed into a single commit automatically when integrating. See OpenJDK Developers’ Guide for more information. |
GHA failure would be fixed with #1268 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi
you add a very thorough description why you backport this.
This should go to the JBS fix comment.
Unfortunately it does not help reviewing the change.
I assume you ran SAP testing on this.
Here my comments:
test/hotspot/jtreg/TEST.ROOT
Copyright. ok.
test/hotspot/jtreg/serviceability/sa/ClhsdbLauncher.java
Why do you remove
processBuilder.redirectError(ProcessBuilder.Redirect.INHERIT);
The special check for OSX is removed in several tests.
It looks different in 11 and the original change, but as
the code is removed this does not matter.
test/hotspot/jtreg/serviceability/sa/TestUniverse.java
Chunk for new test not in 11 omitted
test/hotspot/jtreg/testlibrary_tests/TestMutuallyExclusivePlatformPredicates.java
Besides replaceing shouldSAAttach by isRoot, isSigned OSX is added.
test/jdk/sun/tools/jhsdb/HeapDumpTestWithActiveProcess.java
Chunk is omitted. Test is not in 11.
test/jtreg-ext/requires/VMProps.java
Copyright.
test/lib/jdk/test/lib/Platform.java
Imports were adapted.
isSignedOSX() is added. What is the original change of this?
Please mention in the JBS FixRequest comment that you
include code from that change.
test/lib/jdk/test/lib/SA/SATestUtils.java
Imports were adapted.
Hi Goetz, thanks for the review. Yes, I have the patch running in the SAP tests without regressions, as far as I can tell.
Hm, when I looked at JDK-8234277, I thought it could be a good idea to do the removal in this change already but maybe you're right and we should keep it consistent. I'm actually not sure about the effects so I'll re-add it.
This originally comes from JDK-8238196. Since merely all of the code of that bug is part of this PR as well (either as is or augmented), I'll add JDK-8238196 to this backport, too. |
/issue add 8238196 |
@RealCLanger |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the clarifications. LGTM.
@RealCLanger This change now passes all automated pre-integration checks. After integration, the commit message for the final commit will be:
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 5 new commits pushed to the
Please see this link for an up-to-date comparison between the source branch of this pull request and the ➡️ To integrate this PR with the above commit message to the |
Thanks for review and approval. /integrate |
Going to push as commit e80c7e1.
Your commit was automatically rebased without conflicts. |
@RealCLanger Pushed as commit e80c7e1. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
This is a backport of JDK-8238268. We need it, because the backport of JDK-8215544 has added code that calls
sudo -E
on MacOS without the option-n
. This call might not return when sudo starts prompting for a password. In our CI it leads to hanging processes when subsequent tests call sudo, even with the -n option. This fix rectifies that behavior. And, after all, it improves test coverage on MacOS, so it's a good backport candidate.Progress
Issues
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk11u-dev pull/1269/head:pull/1269
$ git checkout pull/1269
Update a local copy of the PR:
$ git checkout pull/1269
$ git pull https://git.openjdk.org/jdk11u-dev pull/1269/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 1269
View PR using the GUI difftool:
$ git pr show -t 1269
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk11u-dev/pull/1269.diff