[8u] RFR 8229501: jdk/test/lib/testlibrary/ClassFileInstaller.java should match hotspot//test/testlibrary version

[Hohensee, Paul]

Works for us.

Thanks,

Paul

On 9/2/19, 9:00 AM, "Andrew John Hughes" <gnu.andrew at redhat.com> wrote:

    
    
    On 27/08/2019 21:05, Hohensee, Paul wrote:
    > There's perhaps some confusion here. The file to be patched is in the jdk repo, not the hotspot repo, i.e., jdk/test/lib/testlibrary/ClassFileInstaller.java, not hotspot/test/testlibrary/ClassFileInstaller.java. The former is quite small and contains only a "main" method, while the latter is much bigger with much more functionality.
    > 
    > The patch for https://bugs.openjdk.java.net/browse/JDK-8005056  went into hs25 and therefore can't be left over after 8189762.
    
    I don't believe I was suggesting it was. 8189762 was over five years
    after 8005056.
    
     I.e., it patches a few lines in the hotspot version, vis
    > 
    > @@ -45,7 +45,9 @@
    > 
    >              // Create the class file's package directory
    >              Path p = Paths.get(pathName);
    > -            Files.createDirectories(p.getParent());
    > +            if (pathName.contains("/")) {
    > +                Files.createDirectories(p.getParent());
    > +            }
    >              // Create the class file
    >              Files.copy(is, p, StandardCopyOption.REPLACE_EXISTING);
    >          }
    > 
    > The above patched code is what's in the current jdk8u version of hotspot/test/testlibrary/ClassFileInstaller.java.
    > 
    > "hg log" on jdk8u/hotspot/test/testlibrary/ClassFileInstaller produces
    > 
    > changeset:   8710:4141ef4c8ba8
    > user:        vaibhav
    > date:        Thu Jul 26 06:16:09 2018 -0400
    > summary:     8189762: [TESTBUG] Create tests for JDK-8146115 container awareness and resource configuration
    > 
    > changeset:   4665:43083e670adf
    > user:        coleenp
    > date:        Mon May 13 15:37:08 2013 -0400
    > summary:     8005056: NPG: Crash after redefining java.lang.Object
    > 
    > changeset:   4202:1b0dc9f87e75
    > parent:      4175:1048edb5434a
    > user:        mgerdin
    > date:        Tue Feb 19 18:45:49 2013 +0100
    > summary:     8006753: fix failed for JDK-8002415 White box testing API for HotSpot
    > 
    > The 8189762 backport patch is at http://hg.openjdk.java.net/jdk8u/jdk8u/hotspot/rev/4141ef4c8ba8, and updates the hotspot version to be identical to what's in jdk10 and tip. There is no change to the jdk version at jdk/test/lib/testlibrary/ClassFileInstaller.java.
    > 
    > We therefore want to update the jdk version to be the same as the hotspot version, but 8189762 already has a jdk8u backport issue associated with it. Shall we add an openjdk8u232 backport to it that patches the jdk version of ClassFileInstaller.java? We thought that would be confusing, so we filed a new bug to do so, namely https://bugs.openjdk.java.net/browse/JDK-8229501. Which would you prefer?
    
    Neither. There is no reason to manually file backport bugs, as the bug
    system will handle this, and a new bug ID would lose any connection
    between the changes and their original source.
    
    Looking at this again, it appears that the JDK version already has the
    same fix to ClassInstaller.java in 8005056, but slightly different code.
    This is the result of JDK-8027803 [0] [1].
    
    So we actually only need to sync the changes in 8189762.
    
    I suggest:
    
    https://cr.openjdk.java.net/~andrew/openjdk8/8189762-jdk/webrev.01/
    
    which then leaves the difference between the two versions as just the
    variation in the fixes:
    
    $ diff -u test/lib/testlibrary/ClassFileInstaller.java
    ../hotspot/test/testlibrary/ClassFileInstaller.java
    --- test/lib/testlibrary/ClassFileInstaller.java        2019-09-02
    16:44:24.993301954 +0100
    +++ ../hotspot/test/testlibrary/ClassFileInstaller.java 2019-06-04
    23:50:49.082742173 +0100
    @@ -246,9 +246,8 @@
             } else {
                 // Create the class file's package directory
                 Path p = Paths.get(pathName);
    -            Path parent = p.getParent();
    -            if (parent != null) {
    -                Files.createDirectories(parent);
    +            if (pathName.contains("/")) {
    +                Files.createDirectories(p.getParent());
                 }
                 // Create the class file
                 Files.copy(is, p, StandardCopyOption.REPLACE_EXISTING);
    
    I think that can stay. I actually think the JDK fix is a little nicer.
    
    If you're happy with that, I'll push it.
    
    > 
    > Thanks,
    > 
    > Paul
    > 
    
    [0] https://bugs.openjdk.java.net/browse/JDK-8027803
    [1] https://hg.openjdk.java.net/jdk8/jdk8/jdk/rev/c04e46dbfea8
    
    Thanks,
    -- 
    Andrew :)
    
    Senior Free Java Software Engineer
    Red Hat, Inc. (http://www.redhat.com)
    
    PGP Key: ed25519/0xCFDA0F9B35964222 (hkp://keys.gnupg.net)
    Fingerprint = 5132 579D D154 0ED2 3E04  C5A0 CFDA 0F9B 3596 4222
    https://keybase.io/gnu_andrew