Sling
  1. Sling
  2. SLING-2448

org.apache.sling.jcr.resource should embed com.google.common.collect.MapDifference

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: JCR Resource 2.0.10
    • Fix Version/s: JCR Resource 2.1.0
    • Component/s: JCR
    • Labels:
      None

      Description

      org.apache.sling.jcr.resource embeds parts of the guava bundle inconsistently. It embeds the Map class, but not the MapDifference interface, which is directly refered from the Maps class.

      This error is typically flagged in Eclipse with a very cryptic message: 'The project was not built since its build path is incomplete. Cannot find the class file for com.google.common.collect.MapDifference$ValueDifference. Fix the build path then try building this project'.

        Activity

        rmuntean created issue -
        rmuntean made changes -
        Field Original Value New Value
        Attachment SLING-2448.txt [ 12519652 ]
        Hide
        rmuntean added a comment -

        Patch which includes the MapDifference class in the embedded resources.

        Show
        rmuntean added a comment - Patch which includes the MapDifference class in the embedded resources.
        Hide
        Justin Edelson added a comment -

        Can you explain how this manifests itself? The use of Guava's Maps class (in fact the use of Guava at all) is an internal implementation detail of this bundle and is not exposed to consumers. Only the Guava classes which are actually required by the bundle are embedded.

        Adding this extra class isn't that big a deal, but it looks like unnecessary bloat.

        Show
        Justin Edelson added a comment - Can you explain how this manifests itself? The use of Guava's Maps class (in fact the use of Guava at all) is an internal implementation detail of this bundle and is not exposed to consumers. Only the Guava classes which are actually required by the bundle are embedded. Adding this extra class isn't that big a deal, but it looks like unnecessary bloat.
        Hide
        rmuntean added a comment -

        In reply to comment #2:
        > Can you explain how this manifests itself? The use of Guava's Maps class (in
        > fact the use of Guava at all) is an internal implementation detail of this
        > bundle and is not exposed to consumers. Only the Guava classes which are
        > actually required by the bundle are embedded.

        The issue here is how Eclipse handles classes found in attached libraries rather than in compilation using javac or runtime usage. Eclipse analyses the Maps class and notices that one of the methods references the MadDifference interface. It tries to resolve it but fails and therefore generates a compilation error.

        I suspect that compiling the bundle using ecj would generate the same error , but I did not check that.

        I will attach a screenshot of what I usually get in Eclipse for this error.

        Show
        rmuntean added a comment - In reply to comment #2: > Can you explain how this manifests itself? The use of Guava's Maps class (in > fact the use of Guava at all) is an internal implementation detail of this > bundle and is not exposed to consumers. Only the Guava classes which are > actually required by the bundle are embedded. The issue here is how Eclipse handles classes found in attached libraries rather than in compilation using javac or runtime usage. Eclipse analyses the Maps class and notices that one of the methods references the MadDifference interface. It tries to resolve it but fails and therefore generates a compilation error. I suspect that compiling the bundle using ecj would generate the same error , but I did not check that. I will attach a screenshot of what I usually get in Eclipse for this error.
        rmuntean made changes -
        Attachment eclipse-errors.png [ 12519923 ]
        Hide
        Felix Meschberger added a comment -

        I think adding this class to the bundle does not hurt (it does not make the bundle substantially larger).

        The bigger problem, I have with the inclusion of the Guava parts themselves, because they make the bundle substantially larger (~25% !).

        Show
        Felix Meschberger added a comment - I think adding this class to the bundle does not hurt (it does not make the bundle substantially larger). The bigger problem, I have with the inclusion of the Guava parts themselves, because they make the bundle substantially larger (~25% !).
        Hide
        rmuntean added a comment -

        There are efforts underway to make the Guava bundle OSGi-fied out of the box , see Guava issue 688 . Since this is a general-purpose library, we might consider not embedding it and making it a dependency.. The total size for the r09 jar is 1.1 MB though, and if size is a concern this is not an attractive option.

        Show
        rmuntean added a comment - There are efforts underway to make the Guava bundle OSGi-fied out of the box , see Guava issue 688 . Since this is a general-purpose library, we might consider not embedding it and making it a dependency.. The total size for the r09 jar is 1.1 MB though, and if size is a concern this is not an attractive option.
        Hide
        Carsten Ziegeler added a comment -

        I'm wondering if we need the functionality at all as we're just using very little of the whole lib.
        Having Guava as a bundle would solve the potential size problem but would add another (large) lib to the list of required bundles

        Show
        Carsten Ziegeler added a comment - I'm wondering if we need the functionality at all as we're just using very little of the whole lib. Having Guava as a bundle would solve the potential size problem but would add another (large) lib to the list of required bundles
        rmuntean made changes -
        Attachment SLING-2448-remove-guava.txt [ 12521137 ]
        Hide
        rmuntean added a comment - - edited

        Here's a patch with removes the guava dependency completely. I've ran mvn clean verify in the bundle directory and all tests pass.

        Show
        rmuntean added a comment - - edited Here's a patch with removes the guava dependency completely. I've ran mvn clean verify in the bundle directory and all tests pass.
        Hide
        Carsten Ziegeler added a comment -

        Thanks Robert, lgtm - if noone objects I'll commit this patch in the next days.

        Show
        Carsten Ziegeler added a comment - Thanks Robert, lgtm - if noone objects I'll commit this patch in the next days.
        Carsten Ziegeler made changes -
        Assignee Carsten Ziegeler [ cziegeler ]
        Carsten Ziegeler committed 1325138 (2 files)
        Reviews: none

        SLING-2448 : org.apache.sling.jcr.resource should embed com.google.common.collect.MapDifference. Apply patch from Robert Munteanu

        Hide
        Carsten Ziegeler added a comment -

        Thanks for your patch Robert, I've applied it in revision 1325138

        Show
        Carsten Ziegeler added a comment - Thanks for your patch Robert, I've applied it in revision 1325138
        Carsten Ziegeler made changes -
        Status Open [ 1 ] Resolved [ 5 ]
        Fix Version/s JCR Resource 2.1.0 [ 12316202 ]
        Resolution Fixed [ 1 ]
        Carsten Ziegeler made changes -
        Status Resolved [ 5 ] Closed [ 6 ]
        Gavin made changes -
        Workflow no-reopen-closed,doc-test-required [ 12659339 ] Copy of no-reopen-closed,doc-test-required [ 12762553 ]
        Gavin made changes -
        Workflow Copy of no-reopen-closed,doc-test-required [ 12762553 ] no-reopen-closed,doc-test-required [ 12765575 ]
        Gavin made changes -
        Workflow no-reopen-closed,doc-test-required [ 12765575 ] re-open possible,doc-test-required [ 12787467 ]
        Hide
        Gavin added a comment -

        temporary re-open so I can migrate the reporter account

        Show
        Gavin added a comment - temporary re-open so I can migrate the reporter account
        Gavin made changes -
        Status Closed [ 6 ] Reopened [ 4 ]
        Gavin made changes -
        Reporter Robert Munteanu [ rmuntean ] Robert Munteanu [ rombert ]
        Hide
        Gavin added a comment -

        reporter has been migrated, closing again.

        Show
        Gavin added a comment - reporter has been migrated, closing again.
        Gavin made changes -
        Status Reopened [ 4 ] Resolved [ 5 ]
        Gavin made changes -
        Status Resolved [ 5 ] Closed [ 6 ]
        Gavin made changes -
        Workflow re-open possible,doc-test-required [ 12787467 ] no-reopen-closed,doc-test-required [ 12790365 ]

          People

          • Assignee:
            Carsten Ziegeler
            Reporter:
            Robert Munteanu
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development