Derby
  1. Derby
  2. DERBY-1046

JVMInfo is duplicated in derbyclient.jar

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 10.2.1.6
    • Fix Version/s: 10.8.2.2, 10.9.1.0
    • Component/s: Build tools
    • Labels:
      None
    • Issue & fix info:
      Patch Available

      Description

      The JVMInfo class is included twice in derbyclient.jar, as
      org.apache.derby.iapi.services.info.JVMInfo and
      org.apache.derby.shared.common.info.JVMInfo. The only one of them
      actually used by the client code is the one found in
      org.apache.derby.shared.common.info.

      org.apache.derby.iapi.services.info.JVMInfo is also included in
      derby.jar, so one could run into problems if the classpath contains
      derbyclient.jar and derby.jar with different versions.

      1. derby-1046.diff
        1 kB
        Andrew McIntyre
      2. derby-1046_diff.txt
        3 kB
        Kathey Marsden
      3. derby-1046_derby-5431_trunk_diff.txt
        4 kB
        Kathey Marsden
      4. derby-1046_derby-5431_10_8_diff.txt
        4 kB
        Kathey Marsden
      5. derby-1046_derby-5431_10_8_diff.txt
        9 kB
        Kathey Marsden
      6. derby-1046_derby-5431_trunk_diff.txt
        11 kB
        Kathey Marsden

        Issue Links

          Activity

          Hide
          Andrew McIntyre added a comment -

          Patch for this issue attached. Just need to exclude JVMInfo from the fileset, since it included all of iapi/services/info. Might want to consider moving to use classlister to generate a dependency list, but that also may be more trouble than its worth.

          Show
          Andrew McIntyre added a comment - Patch for this issue attached. Just need to exclude JVMInfo from the fileset, since it included all of iapi/services/info. Might want to consider moving to use classlister to generate a dependency list, but that also may be more trouble than its worth.
          Hide
          David Van Couvering added a comment -

          It looks like ProductVersionHolder has the same potential issues around shadowing as JVMInfo. Perhaps we should (for now) make a copy of this class as well into shared and exclude iapi/services/ProductVersionHolder from the client jar file.

          Show
          David Van Couvering added a comment - It looks like ProductVersionHolder has the same potential issues around shadowing as JVMInfo. Perhaps we should (for now) make a copy of this class as well into shared and exclude iapi/services/ProductVersionHolder from the client jar file.
          Hide
          Andrew McIntyre added a comment -

          Except that, unlike JVMInfo, ProductVersionHolder and the other files in iapi.services.info haven't changed from 10.1 (yet). Seems like it would be ok to leave them for now.

          Show
          Andrew McIntyre added a comment - Except that, unlike JVMInfo, ProductVersionHolder and the other files in iapi.services.info haven't changed from 10.1 (yet). Seems like it would be ok to leave them for now.
          Hide
          Knut Anders Hatlen added a comment -

          Seems like this has been fixed. Closing the issue.

          Show
          Knut Anders Hatlen added a comment - Seems like this has been fixed. Closing the issue.
          Hide
          Kathey Marsden added a comment -

          This issue was closed as fixed but the patch was actually not checked in. It looks like sysinfo does use JVMInfo at least in 10.8, but doesn't need too really, so perhaps Andrew's solution of excluding JVMInfo is a good one but some other minor changes to sysinfo would also be needed.

          Show
          Kathey Marsden added a comment - This issue was closed as fixed but the patch was actually not checked in. It looks like sysinfo does use JVMInfo at least in 10.8, but doesn't need too really, so perhaps Andrew's solution of excluding JVMInfo is a good one but some other minor changes to sysinfo would also be needed.
          Hide
          Kathey Marsden added a comment -

          Attached is a patch for this issue which removes the call to JVMInfo.derbyVMLevel() in tools.sysinfo.Main and replaces it with a local method. The new local method is simpler than the initialization in JVMInfo, but I think it is sufficient for sysinfo.

          I tried sysinfo with the with IBM and Sun JDK's 1.4.2, 1.5, 1.6, and 1.7 as well as weme 6.2. I verified that it fixes DERBY-5431. I am running tests now.

          I would appreciate if someone could try sysinfo with patch with phoneME and Oracle embedded ME client.

          Show
          Kathey Marsden added a comment - Attached is a patch for this issue which removes the call to JVMInfo.derbyVMLevel() in tools.sysinfo.Main and replaces it with a local method. The new local method is simpler than the initialization in JVMInfo, but I think it is sufficient for sysinfo. I tried sysinfo with the with IBM and Sun JDK's 1.4.2, 1.5, 1.6, and 1.7 as well as weme 6.2. I verified that it fixes DERBY-5431 . I am running tests now. I would appreciate if someone could try sysinfo with patch with phoneME and Oracle embedded ME client.
          Hide
          Knut Anders Hatlen added a comment -

          Perhaps we could just remove that line from the sysinfo output to avoid having to maintain duplicated code? Before, it used to tell exactly what Derby recognized the current platform as and which JDBC level it was running at. Now, it just tells what sysinfo thinks the engine recognized the platform as, and that information could just as easily and reliably be taken directly from the Java Information section of the sysinfo output.

          There's a small typo in the patch: The Java SE 7 string lacks a hyphen (for consistency with the other versions).

          Show
          Knut Anders Hatlen added a comment - Perhaps we could just remove that line from the sysinfo output to avoid having to maintain duplicated code? Before, it used to tell exactly what Derby recognized the current platform as and which JDBC level it was running at. Now, it just tells what sysinfo thinks the engine recognized the platform as, and that information could just as easily and reliably be taken directly from the Java Information section of the sysinfo output. There's a small typo in the patch: The Java SE 7 string lacks a hyphen (for consistency with the other versions).
          Hide
          Rick Hillegas added a comment -

          Thanks for the patch, Kathey. Before applying it, I saw JVMInfo in the following jar files:

          derby.jar
          derbyclient.jar
          derbytools.jar

          After applying the patch, I see JVMInfo in the following jar files:

          derby.jar
          derbytools.jar

          In all cases, JVMInfo appears in this directory:

          org/apache/derby/iapi/services/info

          So this patch achieves what this JIRA wants to accomplish. I believe that the presence of JVMInfo in derbytools.jar means that DERBY-5431 is still possible.

          I would recommend the following improvement to this patch: The cloned code warns the user to propagate changes to JVMInfo as well. I think that there should be a similar warning in JVMInfo, advising people to propagate changes to org.apache.derby.impl.tools.sysinfo.Main.

          Thanks,
          -Rick

          Show
          Rick Hillegas added a comment - Thanks for the patch, Kathey. Before applying it, I saw JVMInfo in the following jar files: derby.jar derbyclient.jar derbytools.jar After applying the patch, I see JVMInfo in the following jar files: derby.jar derbytools.jar In all cases, JVMInfo appears in this directory: org/apache/derby/iapi/services/info So this patch achieves what this JIRA wants to accomplish. I believe that the presence of JVMInfo in derbytools.jar means that DERBY-5431 is still possible. I would recommend the following improvement to this patch: The cloned code warns the user to propagate changes to JVMInfo as well. I think that there should be a similar warning in JVMInfo, advising people to propagate changes to org.apache.derby.impl.tools.sysinfo.Main. Thanks, -Rick
          Hide
          Rick Hillegas added a comment -

          After applying the patch, I ran sysinfo on phoneME. It produced the following version info, which looks right to me:

          JRE - JDBC: J2ME - JDBC for CDC/FP 1.1

          Thanks,
          -Rick

          Show
          Rick Hillegas added a comment - After applying the patch, I ran sysinfo on phoneME. It produced the following version info, which looks right to me: JRE - JDBC: J2ME - JDBC for CDC/FP 1.1 Thanks, -Rick
          Hide
          Kathey Marsden added a comment -

          Thank you Knut for your comments.
          II think it is fine to remove the line in trunk, but was hesitant to do so in 10.8.2 as we recently had a report from a user reporting the JDBC version wasn't what he expected, so I was concerned that perhaps he/others are parsing sysinfo for this information. I will make a separate trunk patch and release note for trunk but keep the duplicate code in the 10.8 branch. Also will fix the typo.

          Thank you Rick for your comments. I will definitely look at the derbytools issue as well. Thank you for catching tha. I will also add the comment to JVMInfo for the 10.8 patch which will keep some duplicate code.

          Show
          Kathey Marsden added a comment - Thank you Knut for your comments. II think it is fine to remove the line in trunk, but was hesitant to do so in 10.8.2 as we recently had a report from a user reporting the JDBC version wasn't what he expected, so I was concerned that perhaps he/others are parsing sysinfo for this information. I will make a separate trunk patch and release note for trunk but keep the duplicate code in the 10.8 branch. Also will fix the typo. Thank you Rick for your comments. I will definitely look at the derbytools issue as well. Thank you for catching tha. I will also add the comment to JVMInfo for the 10.8 patch which will keep some duplicate code.
          Hide
          Kathey Marsden added a comment -

          Sorry previous upload had some cruft from another patch.
          Here is a patch for DERBY-1046 and DERBY-5431. It removes the line with JDBC information from sysinfo and also removes JDKInfo calls from the xahelper class which lives in derbytools.jar. This is for trunk. For 10.8 we will keep the line in case someone is using it .

          Show
          Kathey Marsden added a comment - Sorry previous upload had some cruft from another patch. Here is a patch for DERBY-1046 and DERBY-5431 . It removes the line with JDBC information from sysinfo and also removes JDKInfo calls from the xahelper class which lives in derbytools.jar. This is for trunk. For 10.8 we will keep the line in case someone is using it .
          Hide
          Kathey Marsden added a comment -

          Here is the 10.8 patch with the changes from the comments from the first patch.

          Show
          Kathey Marsden added a comment - Here is the 10.8 patch with the changes from the comments from the first patch.
          Hide
          Knut Anders Hatlen added a comment -

          On Java 5 in my environment, the following code throws an UnsupportedClassVersionError, not a ClassNotFoundException:

          Class.forName("org.apache.derby.jdbc.ClientXADataSource40");

          So I think the xaHelper changes on trunk won't work as expected.

          Apart from that, the patches look fine to me.

          Show
          Knut Anders Hatlen added a comment - On Java 5 in my environment, the following code throws an UnsupportedClassVersionError, not a ClassNotFoundException: Class.forName("org.apache.derby.jdbc.ClientXADataSource40"); So I think the xaHelper changes on trunk won't work as expected. Apart from that, the patches look fine to me.
          Hide
          Kathey Marsden added a comment -

          Here is an updated 10.8 patch with changes review comments. Fixes xaHelper on < JDK 1.6. Running tests.

          Show
          Kathey Marsden added a comment - Here is an updated 10.8 patch with changes review comments. Fixes xaHelper on < JDK 1.6. Running tests.
          Hide
          Kathey Marsden added a comment -

          Here is the trunk patch which removes the JRE/JDBC line all together. The JDBCMBean test was comparing the driver level to sysinfo. That test was changed to compare to DatabaseMetaData JDBC version values.

          Show
          Kathey Marsden added a comment - Here is the trunk patch which removes the JRE/JDBC line all together. The JDBCMBean test was comparing the driver level to sysinfo. That test was changed to compare to DatabaseMetaData JDBC version values.

            People

            • Assignee:
              Kathey Marsden
              Reporter:
              Knut Anders Hatlen
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development