commons-compress icon indicating copy to clipboard operation
commons-compress copied to clipboard

Remove Commons Lang dependency

Open ppkarwasz opened this issue 1 year ago • 5 comments

Commons Compress version 1.26.0 depends on Commons Lang, which is a 650 KiB dependency. In reality, however, only 4 methods accounting for less than 1% of Commons Lang are used.

We replace the dependency on commons-lang3 with a local copy of these 4 methods.

ppkarwasz avatar Nov 17 '24 16:11 ppkarwasz

As I've pointed out on the dev mailing list: This a horrible mess IMO. To make things even worse, this PR adds public code (the package name is irrelevant), with ZERO Javadoc and ZERO unit tests. This is not the example we should set for contributors and even less for ourselves. This seems to me like a reflection of short term hacking without long term care. I am -1, but it seems pointless as some people are in love with copy-paste over long term maintenance.

garydgregory avatar Nov 18 '24 12:11 garydgregory

To make things even worse, this PR adds public code (the package name is irrelevant), with ZERO Javadoc and ZERO unit tests.

Sure, there is work to be done on this PR (I marked it as draft). Don't worry, if you are willing to accept this solution, I will make the package really private (in OSGi and JPMS) and add the relevant Javadoc and unit tests.

This is not the example we should set for contributors and even less for ourselves. This seems to me like a reflection of short term hacking without long term care. I am -1, but it seems pointless as some people are in love with copy-paste over long term maintenance.

Let's ignore FieldUtils for now. What kind of maintenance work do you think this code will require in the following years? As I see it:

  • The usages of ArrayFill in Commons Compress are overkill: all the usages provide a non-null array parameter, so the method can be simplified to:
    public int[] fill(int[] a, int val) {
      Arrays.fill(a, val);
      return a;
    }
    
    Also note that Commons Lang does not provide a helper method for the most common usage of Arrays.fill, which is:
    public int[] createFilledArray(int size, int val) {
      int[] a = new int[size];
      Arrays.fill(a, val);
      return a;
    }
    
  • The implementation of SystemProperties.getOsName() in Commons Lang delegates to a complex wrapper to System.getProperty(name) that handles security exceptions and an empty or null parameter name. None of these conditions apply to the os.name property, which must have a valuet (see Javadoc for System.getProperties() and should be allowed by SecurityManager.

FieldUtils

The code that relies on FieldUtils constitutes a maintenance problem itself, since it relies on the protected FilterInputStream.in field. The field is protected for a reason: users of FilterInputStream should not skip access the underlying stream directly, but use the wrapping FilterInputStream instead.

ppkarwasz avatar Nov 18 '24 13:11 ppkarwasz

I released Kala Compress 1.27.1-1. It is based on commons-compress, but does not depend on commons-io and commons-lang. Maybe this will be what you want.

Glavo avatar Dec 03 '24 12:12 Glavo

I released Kala Compress 1.27.1-1. It is based on commons-compress, but does not depend on commons-io and commons-lang. Maybe this will be what you want.

I don't believe that forking Commons Compress is the right approach. Your changes in Kala Compress are aligned with what we plan for Commons Compress 2.x.

I have created a 2.x branch. Feel free to open PRs against that branch to split Commons Compress in separate modules. Probably the first module we need to split is a commons-compress2-api module that would include the interfaces to access the various compression algorithms.

ppkarwasz avatar Dec 04 '24 10:12 ppkarwasz

We would really appreciate this one, it came as a surprise to us that when we updated from 1.25.0 we had to add a new dependency on commons lang.

acasademont avatar Mar 25 '25 15:03 acasademont