Velocity
  1. Velocity
  2. VELOCITY-789

Reorganize dependencies in Velocity Engine Core

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.x
    • Fix Version/s: 2.x
    • Component/s: Engine
    • Labels:
      None

      Description

      Currently Velocity Engine core depends on:
      Commons Lang
      Commons Collections
      Avalon LogKit
      Apache Jakarta ORO

      LogKit and ORO should be removed, as they are obsolete. Commons Lang and Collections should be replaced or shaded.

        Issue Links

          Activity

          Hide
          Antonio Petrelli added a comment -

          Commons Lang and Commons Collections have been shaded.
          Original artifact: 380,182 bytes
          Shaded artifact: 441.062
          Not too bad

          Show
          Antonio Petrelli added a comment - Commons Lang and Commons Collections have been shaded. Original artifact: 380,182 bytes Shaded artifact: 441.062 Not too bad
          Hide
          Antonio Petrelli added a comment -

          Adrian, go on with it, I think that a common conversion utility, already stable and mature (just like StringEscapeUtils is mature) is better than any copied code.
          You know, people change their minds sometimes :-D

          Show
          Antonio Petrelli added a comment - Adrian, go on with it, I think that a common conversion utility, already stable and mature (just like StringEscapeUtils is mature) is better than any copied code. You know, people change their minds sometimes :-D
          Hide
          Adrian Tarau added a comment -

          Shading I'm thinking to add a dependency to BeanUtils's converters package for type conversion in Configuration. This will add 47kb in compressed byte code but I think it's worth it...if not approved I will go with a simpler type conversion.

          http://commons.apache.org/beanutils/v1.8.3/apidocs/index.html?org/apache/commons/beanutils/converters/package-summary.html

          Show
          Adrian Tarau added a comment - Shading I'm thinking to add a dependency to BeanUtils's converters package for type conversion in Configuration. This will add 47kb in compressed byte code but I think it's worth it...if not approved I will go with a simpler type conversion. http://commons.apache.org/beanutils/v1.8.3/apidocs/index.html?org/apache/commons/beanutils/converters/package-summary.html
          Hide
          Antonio Petrelli added a comment -

          The problem is only Commons Lang, because Commons Collections is not useful anymore, because I think Adrian will remove the only class really used: ExtendedProperties.

          About Commons Lang, at a first glance I noticed that the really needed classes are StringEscapeUtils and StringUtils.
          But, about StringUtils, we need only "lame" methods (isEmpty, isNotEmpty, equals) that can be easily copied into the original StringUtils inside Velocity itself.

          Now it's time of StringEscapeUtils. It is really useful, with the exception of escapeSql, that is pretty lame too (it replaces quotes with two quotes, nothing more). This method is problematic since it uses StringUtils.replace, that works differently to String.replaceAll (no regex allowed).

          So I can try to shade StringEscapeUtils, Entities and UnhandledException, and let's see what happens

          Show
          Antonio Petrelli added a comment - The problem is only Commons Lang, because Commons Collections is not useful anymore, because I think Adrian will remove the only class really used: ExtendedProperties. About Commons Lang, at a first glance I noticed that the really needed classes are StringEscapeUtils and StringUtils. But, about StringUtils, we need only "lame" methods (isEmpty, isNotEmpty, equals) that can be easily copied into the original StringUtils inside Velocity itself. Now it's time of StringEscapeUtils. It is really useful, with the exception of escapeSql, that is pretty lame too (it replaces quotes with two quotes, nothing more). This method is problematic since it uses StringUtils.replace, that works differently to String.replaceAll (no regex allowed). So I can try to shade StringEscapeUtils, Entities and UnhandledException, and let's see what happens
          Hide
          Adrian Tarau added a comment -

          I'm pro-shading since you are linked to their released versions(get fixes). If some modifications are need it(third-party API doesn't allow the integration without changing the code) that means Velocity doesn't need that library and it's best to implement something internal.

          Show
          Adrian Tarau added a comment - I'm pro-shading since you are linked to their released versions(get fixes). If some modifications are need it(third-party API doesn't allow the integration without changing the code) that means Velocity doesn't need that library and it's best to implement something internal.
          Hide
          Nathan Bubna added a comment -

          Why not shade? At least with shading we can get updates and pass off bug reports more easily, right?

          Show
          Nathan Bubna added a comment - Why not shade? At least with shading we can get updates and pass off bug reports more easily, right?
          Hide
          Antonio Petrelli added a comment -

          Just an update.
          I decided not to shade dependencies, but to copy the code and edit them a bit.
          In fact, all dependencies are from Apache products, so we can use it as it was ours (we are, after all, the Apache Software Foundation ).

          Show
          Antonio Petrelli added a comment - Just an update. I decided not to shade dependencies, but to copy the code and edit them a bit. In fact, all dependencies are from Apache products, so we can use it as it was ours (we are, after all, the Apache Software Foundation ).
          Hide
          Adrian Tarau added a comment -

          For a library that has ~500kb bringing another 500kb for a few classes it's just ... not optimal. Don't get me wrong I am not against using great code in Velocity but this days it is very common to shade libraries to reduce the size and avoid version conflicts(include only what you use).

          Since Velocity is moving to Maven, Maven Shade Plugin( http://maven.apache.org/plugins/maven-shade-plugin) can be used to include(and relocate) any class dependencies.

          The only disadvantage I would see is to get bug reports for classes that are not Velocity specific(but since those classes will be relocated under org.apache.velocity it will look like they are developed as part of Velocity project), but I think this would not happen to often.

          Show
          Adrian Tarau added a comment - For a library that has ~500kb bringing another 500kb for a few classes it's just ... not optimal. Don't get me wrong I am not against using great code in Velocity but this days it is very common to shade libraries to reduce the size and avoid version conflicts(include only what you use). Since Velocity is moving to Maven, Maven Shade Plugin( http://maven.apache.org/plugins/maven-shade-plugin ) can be used to include(and relocate) any class dependencies. The only disadvantage I would see is to get bug reports for classes that are not Velocity specific(but since those classes will be relocated under org.apache.velocity it will look like they are developed as part of Velocity project), but I think this would not happen to often.
          Hide
          Jarkko Viinamäki added a comment -

          "Commons Lang and Collections should be replaced or shaded."

          Why?

          Show
          Jarkko Viinamäki added a comment - "Commons Lang and Collections should be replaced or shaded." Why?

            People

            • Assignee:
              Antonio Petrelli
              Reporter:
              Antonio Petrelli
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development