RFR(M/T) : 8243935 : s.h.WhiteBoxPermission must be ClassFileInstall-ed together w/ s.h.WhiteBox

Igor Ignatyev igor.ignatyev at oracle.com
Wed Apr 29 02:58:42 UTC 2020 

Thanks for review David, I really appreciate you spending time on it.

sure-sure, 545 out of 638 files got copyrights update before the push. 

Thanks,
-- Igor

> On Apr 28, 2020, at 7:31 PM, David Holmes <david.holmes at oracle.com> wrote:
> 
> Hi Igor,
> 
> On 29/04/2020 10:06 am, Igor Ignatyev wrote:
>> update:
>>  - changed the bug's title to 'remove copying of s.h.WB$WhiteBoxPermission in hotspot tests'
>>  - filed JDK-8244052 for test/jdk tests
>>  - run hs-tier[1-3] and test/hotspot/jtreg/containers which accounts for all changed tests, all tests have passed
>> so now I'm just one LGTM short from pushing this.
> 
> I can't say I think this was a particularly worthwhile exercise, but FWIW LGTM.
> 
> However, you will also need to ensure all copyrights are updated if needed.
> 
> Thanks,
> David
> 
>> -- Igor
>>> On Apr 28, 2020, at 2:56 PM, Igor Ignatyev <igor.ignatyev at oracle.com> wrote:
>>> 
>>> Thanks Stefan.
>>> 
>>> I'm going to update other test suites by separate RFEs.
>>> 
>>> -- Igor
>>> 
>>>> On Apr 28, 2020, at 1:31 PM, Stefan Karlsson <stefan.karlsson at oracle.com> wrote:
>>>> 
>>>> Looks good.
>>>> 
>>>> Ran hg diff -r -2 -U 0 and did a quick scan.
>>>> 
>>>> Are you going to fix occurrences in test/jdk, or do you leave that as an exercise for others?
>>>> 
>>>> Thanks,
>>>> StefanK
>>>> 
>>>> On 2020-04-28 18:23, Igor Ignatyev wrote:
>>>>> uploaded http://cr.openjdk.java.net/~iignatyev/8243935/8243935.01.patch
>>>>> 
>>>>> -- Igor
>>>>> 
>>>>>> On Apr 28, 2020, at 9:18 AM, Stefan Karlsson <stefan.karlsson at oracle.com> wrote:
>>>>>> 
>>>>>> On 2020-04-28 18:16, Igor Ignatyev wrote:
>>>>>>> Hi Stefan,
>>>>>>> 
>>>>>>> I guess you are interested only in an actual patch, if so I would rather just use hg diff to get the patch and attach/upload it b/c, by some reasons, webrev takes a lot of time to be generated.
>>>>>> Sure. hg diff works.
>>>>>> 
>>>>>> StefanK
>>>>>> 
>>>>>>> -- Igor
>>>>>>> 
>>>>>>>> On Apr 28, 2020, at 8:57 AM, Stefan Karlsson <stefan.karlsson at oracle.com> wrote:
>>>>>>>> 
>>>>>>>> Hi Igor,
>>>>>>>> 
>>>>>>>> On 2020-04-28 17:30, Igor Ignatyev wrote:
>>>>>>>>> Hi David, Stefan,
>>>>>>>>> 
>>>>>>>>> there is no issue, this patch is just a spin-off of other patch I was working on. I noticed that some tests copy WhiteBoxPermission and other don't, so I decided to clean this up. I completely forgot about the hack we did in ClassFileInstaller (I guess partially b/c we still have tests which copy WhiteBoxPermission explicitly) so now I think it's better to just remove all explicit copying of WhiteBox$WhiteBoxPermission (which kinda echoes Stefan's patch). this of course touches more files, on the bright side this makes them all neater. so I run sed '/^\s*\*\s*sun.hotspot.WhiteBox\$WhiteBoxPermission/'d to remove all WhiteBoxPermission, and them removed some leftovers in 'containers/docker':
>>>>>>>>> - full webrev:
>>>>>>>>>  http://cr.openjdk.java.net/~iignatyev//8243935/webrev.01/
>>>>>>>> Your open.patch is incorrectly generated and reflects the earlier patch:
>>>>>>>> https://cr.openjdk.java.net/~iignatyev//8243935/webrev.01/open.changeset
>>>>>>>> 
>>>>>>>> It's a bug in webrev, but I don't remember under what circumstances it occurs. Could you rerun with: -r qparent -c 8243935 -C -N and see if it works better?
>>>>>>>> 
>>>>>>>> Thanks,
>>>>>>>> StefanK
>>>>>>>> 
>>>>>>>>>> 656 lines changed: 0 ins; 612 del; 44 mod;
>>>>>>>>> - incremental webrev:
>>>>>>>>>  http://cr.openjdk.java.net/~iignatyev//8243935/webrev.0-1/
>>>>>>>>>> 722 lines changed: 0 ins; 719 del; 3 mod;
>>>>>>>>> if we agree that this is the way to go, I'll repurpose 8243935 to remove WhiteBoxPermission (and obviously will retest the patch).
>>>>>>>>> 
>>>>>>>>> Thanks,
>>>>>>>>> -- Igor
>>>>>>>>> 
>>>>>>>>> 
>>>>>>>>>> On Apr 28, 2020, at 2:49 AM, David Holmes <david.holmes at oracle.com> wrote:
>>>>>>>>>> 
>>>>>>>>>> On 28/04/2020 5:34 pm, Stefan Karlsson wrote:
>>>>>>>>>>> Hi,
>>>>>>>>>>> I have another idea. Can't we simply remove WhiteBoxPermission and incorporate that functionality in the WhiteBox class? We would then be able to remove all these WhiteBoxPermission lines.
>>>>>>>>>> Interesting though a little hackish. :)
>>>>>>>>>> 
>>>>>>>>>> But why is this an issue anyway we already hacked ClassfileInstaller to ensure it always copies the permission class!!!
>>>>>>>>>> 
>>>>>>>>>> 102     // Add commonly used inner classes that are often omitted by mistake. Currently
>>>>>>>>>> 103     // we support only sun.hotspot.WhiteBox$WhiteBoxPermission. See JDK-8199290
>>>>>>>>>> 104     private static String[] addInnerClasses(String[] classes, int startIdx) {
>>>>>>>>>> 105         boolean seenWB = false;
>>>>>>>>>> 106         boolean seenWBInner = false;
>>>>>>>>>> 107         final String wb = "sun.hotspot.WhiteBox";
>>>>>>>>>> 108         final String wbInner = "sun.hotspot.WhiteBox$WhiteBoxPermission";
>>>>>>>>>> 
>>>>>>>>>> David
>>>>>>>>>> -----
>>>>>>>>>> 
>>>>>>>>>>> I've tested this and it seems to work:
>>>>>>>>>>> https://cr.openjdk.java.net/~stefank/prototype/removeWhiteBoxPermission/webrev.01/ Summary of changes:    695 lines changed: 0 ins; 657 del; 38 mod; 60752 unchg
>>>>>>>>>>> The interesting code:
>>>>>>>>>>> https://cr.openjdk.java.net/~stefank/prototype/removeWhiteBoxPermission/webrev.01/test/lib/sun/hotspot/WhiteBox.java.udiff.html (There are some parts of the patch that was butchered by the sed-script - I'll fix this if we agree that this is a good thing to do)
>>>>>>>>>>> If you think this is something we should do, I'll create an RFE, complete the patch, run more tests, and send it out for review.
>>>>>>>>>>> Thanks,
>>>>>>>>>>> StefanK
>>>>>>>>>>> On 2020-04-28 08:08, David Holmes wrote:
>>>>>>>>>>>> On 28/04/2020 3:37 pm, Igor Ignatyev wrote:
>>>>>>>>>>>>>> On Apr 27, 2020, at 10:03 PM, David Holmes <david.holmes at oracle.com> wrote:
>>>>>>>>>>>>>> 
>>>>>>>>>>>>>> Hi Igor,
>>>>>>>>>>>>>> 
>>>>>>>>>>>>>> On 28/04/2020 12:32 pm, Igor Ignatyev wrote:
>>>>>>>>>>>>>>> http://cr.openjdk.java.net/~iignatyev//8243935/webrev.00/
>>>>>>>>>>>>>>>> 338 lines changed: 107 ins; 0 del; 231 mod
>>>>>>>>>>>>>>> Hi all,
>>>>>>>>>>>>>>> could you please review the trivial yet tedious patch which adds 'sun.hotspot.WhiteBox$WhiteBoxPermission' to all 'ClassFileInstaller sun.hotspot.WhiteBox' actions?
>>>>>>>>>>>>>>> the patch also
>>>>>>>>>>>>>>>  - replaces '@run main ClassFileInstaller' w/ '@run driver ClassFileInstaller'
>>>>>>>>>>>>>>>  - unifies the way ClassFileInstaller sun.hotspot.WhiteBox are aligned
>>>>>>>>>>>>>> That alignment change is unnecessary and makes it very hard to review this patch.
>>>>>>>>>>>>>> 
>>>>>>>>>>>>>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8243935
>>>>>>>>>>>>>>> webrevs:
>>>>>>>>>>>>>>>  - http://cr.openjdk.java.net/~iignatyev//8243935/webrev.00/ -- regular webrev
>>>>>>>>>>>>>>>  - http://cr.openjdk.java.net/~iignatyev//8243935/webrev.00.including_ws -- shows "whitespace only" changes
>>>>>>>>>>>>>> I'd like to see the patch that leaves out the whitespace-only changes - is that possible?
>>>>>>>>>>>>> sure, I've used `hg diff -w` (--ignore-all-space    ignore white space when comparing lines) to remove all whitespace changes, here is webrev -- http://cr.openjdk.java.net/~iignatyev//8243935/webrev.00.w/
>>>>>>>>>>>> Thanks that was somewhat better. Change seems okay.
>>>>>>>>>>>> 
>>>>>>>>>>>> David
>>>>>>>>>>>> 
>>>>>>>>>>>>> -- Igor
>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>> David
>>>>>>>>>>>>>> 
>>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>>> -- Igor
>>>> 
>>>

More information about the hotspot-dev mailing list