You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@commons.apache.org by Gary Gregory <ga...@gmail.com> on 2024/10/24 17:25:32 UTC
Re: (commons-compress) branch master updated: Removed the runtime dependency on commons-lang3
-1.
No copypasta, please. I want to reduce code patterns that are duplicated
all over the place.
Gary
On Thu, Oct 24, 2024 at 1:15 PM <eb...@apache.org> wrote:
> This is an automated email from the ASF dual-hosted git repository.
>
> ebourg pushed a commit to branch master
> in repository https://gitbox.apache.org/repos/asf/commons-compress.git
>
>
> The following commit(s) were added to refs/heads/master by this push:
> new 23877f821 Removed the runtime dependency on commons-lang3
> 23877f821 is described below
>
> commit 23877f8216243df6b2f1dddac2a02b8403d2435d
> Author: Emmanuel Bourg <eb...@apache.org>
> AuthorDate: Thu Oct 24 19:13:47 2024 +0200
>
> Removed the runtime dependency on commons-lang3
> ---
> pom.xml | 3 +--
> src/changes/changes.xml | 1 +
> .../compress/archivers/tar/TarArchiveEntry.java | 3 +--
> .../archivers/tar/TarArchiveOutputStream.java | 5 ++--
> .../commons/compress/archivers/zip/BinaryTree.java | 5 ++--
> .../compressors/deflate64/HuffmanDecoder.java | 4 ++--
> .../compressors/lz77support/LZ77Compressor.java | 6 ++---
> .../compress/harmony/unpack200/Archive.java | 2 +-
> .../harmony/unpack200/Pack200UnpackerAdapter.java | 27
> ++++++++++++----------
> .../archivers/tar/TarArchiveOutputStreamTest.java | 4 ++--
> .../archivers/zip/ZipArchiveInputStreamTest.java | 5 ++--
> .../compress/archivers/zip/ZipFileTest.java | 4 ++--
> .../lz4/BlockLZ4CompressorOutputStreamTest.java | 5 ++--
> 13 files changed, 40 insertions(+), 34 deletions(-)
>
> diff --git a/pom.xml b/pom.xml
> index 658864a0e..3ef7c6ebc 100644
> --- a/pom.xml
> +++ b/pom.xml
> @@ -65,8 +65,6 @@ Brotli, Zstandard and ar, cpio, jar, tar, zip, dump, 7z,
> arj.
> javax.crypto.*;resolution:=optional,
> org.apache.commons.io;resolution:=optional,
> org.apache.commons.io.*;resolution:=optional,
> - org.apache.commons.lang3;resolution:=optional,
> - org.apache.commons.lang3.reflect;resolution:=optional,
> org.apache.commons.codec;resolution:=optional,
> org.apache.commons.codec.digest;resolution:=optional,
> *
> @@ -213,6 +211,7 @@ Brotli, Zstandard and ar, cpio, jar, tar, zip, dump,
> 7z, arj.
> <groupId>org.apache.commons</groupId>
> <artifactId>commons-lang3</artifactId>
> <version>3.17.0</version>
> + <scope>test</scope>
> </dependency>
> </dependencies>
>
> diff --git a/src/changes/changes.xml b/src/changes/changes.xml
> index cb7711e3d..4bfd3dd0f 100644
> --- a/src/changes/changes.xml
> +++ b/src/changes/changes.xml
> @@ -48,6 +48,7 @@ The <action> type attribute can be add,update,fix,remove.
> <!-- FIX -->
> <action type="fix" issue="COMPRESS-686" dev="ggregory"
> due-to="Richard Blank, Gary Gregory">Better exception messages in
> SeekableInMemoryByteChannel.</action>
> <action type="fix" dev="ggregory" due-to="yujincheng08, Gary
> Gregory">ZipArchiveOutputStream.addRawArchiveEntry() should check
> is2PhaseSource #571.</action>
> + <action type="fix" dev="ebourg" due-to="Emmanuel Bourg">The runtime
> dependency on commons-lang3 has been removed</action>
> <!-- ADD -->
> <!-- UPDATE -->
> <action type="update" dev="ggregory" due-to="Dependabot, Gary
> Gregory">Bump org.apache.commons:commons-parent from 72 to 78 #563, #567,
> #574, #582, #587, #595.</action>
> diff --git
> a/src/main/java/org/apache/commons/compress/archivers/tar/TarArchiveEntry.java
> b/src/main/java/org/apache/commons/compress/archivers/tar/TarArchiveEntry.java
> index e02eeb3a2..ad64263d9 100644
> ---
> a/src/main/java/org/apache/commons/compress/archivers/tar/TarArchiveEntry.java
> +++
> b/src/main/java/org/apache/commons/compress/archivers/tar/TarArchiveEntry.java
> @@ -53,7 +53,6 @@ import org.apache.commons.compress.utils.IOUtils;
> import org.apache.commons.compress.utils.ParsingUtils;
> import org.apache.commons.compress.utils.TimeUtils;
> import org.apache.commons.io.file.attribute.FileTimes;
> -import org.apache.commons.lang3.SystemProperties;
>
> /**
> * An entry in a <a href="
> https://www.gnu.org/software/tar/manual/html_node/Standard.html">Tar
> archive</a>.
> @@ -229,7 +228,7 @@ public class TarArchiveEntry implements ArchiveEntry,
> TarConstants, EntryStreamO
> */
> private static String normalizeFileName(String fileName, final
> boolean preserveAbsolutePath) {
> if (!preserveAbsolutePath) {
> - final String property = SystemProperties.getOsName();
> + final String property = System.getProperty("os.name");
> if (property != null) {
> final String osName = property.toLowerCase(Locale.ROOT);
>
> diff --git
> a/src/main/java/org/apache/commons/compress/archivers/tar/TarArchiveOutputStream.java
> b/src/main/java/org/apache/commons/compress/archivers/tar/TarArchiveOutputStream.java
> index 4f3588d9b..f84064772 100644
> ---
> a/src/main/java/org/apache/commons/compress/archivers/tar/TarArchiveOutputStream.java
> +++
> b/src/main/java/org/apache/commons/compress/archivers/tar/TarArchiveOutputStream.java
> @@ -32,6 +32,7 @@ import java.nio.file.LinkOption;
> import java.nio.file.Path;
> import java.nio.file.attribute.FileTime;
> import java.time.Instant;
> +import java.util.Arrays;
> import java.util.HashMap;
> import java.util.Map;
>
> @@ -43,7 +44,6 @@ import org.apache.commons.compress.utils.TimeUtils;
> import org.apache.commons.io.Charsets;
> import org.apache.commons.io.file.attribute.FileTimes;
> import org.apache.commons.io.output.CountingOutputStream;
> -import org.apache.commons.lang3.ArrayFill;
>
> /**
> * The TarOutputStream writes a UNIX tar archive as an OutputStream.
> Methods are provided to put entries, and then write their contents by
> writing to this
> @@ -649,7 +649,8 @@ public class TarArchiveOutputStream extends
> ArchiveOutputStream<TarArchiveEntry>
> * Writes an EOF (end of archive) record to the tar archive. An EOF
> record consists of a record of all zeros.
> */
> private void writeEOFRecord() throws IOException {
> - writeRecord(ArrayFill.fill(recordBuf, (byte) 0));
> + Arrays.fill(recordBuf, (byte) 0);
> + writeRecord(recordBuf);
> }
>
> /**
> diff --git
> a/src/main/java/org/apache/commons/compress/archivers/zip/BinaryTree.java
> b/src/main/java/org/apache/commons/compress/archivers/zip/BinaryTree.java
> index c02fce78c..586a0aea7 100644
> ---
> a/src/main/java/org/apache/commons/compress/archivers/zip/BinaryTree.java
> +++
> b/src/main/java/org/apache/commons/compress/archivers/zip/BinaryTree.java
> @@ -22,9 +22,9 @@ package org.apache.commons.compress.archivers.zip;
> import java.io.EOFException;
> import java.io.IOException;
> import java.io.InputStream;
> +import java.util.Arrays;
>
> import org.apache.commons.compress.utils.IOUtils;
> -import org.apache.commons.lang3.ArrayFill;
>
> /**
> * Binary tree of positive values.
> @@ -140,7 +140,8 @@ final class BinaryTree {
> if (depth < 0 || depth > 30) {
> throw new IllegalArgumentException("depth must be bigger than
> 0 and not bigger than 30" + " but is " + depth);
> }
> - tree = ArrayFill.fill(new int[(int) ((1L << depth + 1) - 1)],
> UNDEFINED);
> + tree = new int[(int) ((1L << depth + 1) - 1)];
> + Arrays.fill(tree, UNDEFINED);
> }
>
> /**
> diff --git
> a/src/main/java/org/apache/commons/compress/compressors/deflate64/HuffmanDecoder.java
> b/src/main/java/org/apache/commons/compress/compressors/deflate64/HuffmanDecoder.java
> index 4d77de9ec..33881a69e 100644
> ---
> a/src/main/java/org/apache/commons/compress/compressors/deflate64/HuffmanDecoder.java
> +++
> b/src/main/java/org/apache/commons/compress/compressors/deflate64/HuffmanDecoder.java
> @@ -31,7 +31,6 @@ import java.util.Arrays;
> import org.apache.commons.compress.utils.BitInputStream;
> import org.apache.commons.compress.utils.ByteUtils;
> import org.apache.commons.compress.utils.ExactMath;
> -import org.apache.commons.lang3.ArrayFill;
>
> /**
> * TODO This class can't be final because it is mocked by Mockito.
> @@ -362,7 +361,8 @@ class HuffmanDecoder implements Closeable {
> Arrays.fill(FIXED_LITERALS, 256, 280, 7);
> Arrays.fill(FIXED_LITERALS, 280, 288, 8);
>
> - FIXED_DISTANCE = ArrayFill.fill(new int[32], 5);
> + FIXED_DISTANCE = new int[32];
> + Arrays.fill(FIXED_DISTANCE, 5);
> }
>
> private static BinaryTreeNode buildTree(final int[] litTable) {
> diff --git
> a/src/main/java/org/apache/commons/compress/compressors/lz77support/LZ77Compressor.java
> b/src/main/java/org/apache/commons/compress/compressors/lz77support/LZ77Compressor.java
> index 1feb1e5bc..224065659 100644
> ---
> a/src/main/java/org/apache/commons/compress/compressors/lz77support/LZ77Compressor.java
> +++
> b/src/main/java/org/apache/commons/compress/compressors/lz77support/LZ77Compressor.java
> @@ -19,10 +19,9 @@
> package org.apache.commons.compress.compressors.lz77support;
>
> import java.io.IOException;
> +import java.util.Arrays;
> import java.util.Objects;
>
> -import org.apache.commons.lang3.ArrayFill;
> -
> /**
> * Helper class for compression algorithms that use the ideas of LZ77.
> *
> @@ -286,7 +285,8 @@ public class LZ77Compressor {
> final int wSize = params.getWindowSize();
> window = new byte[wSize * 2];
> wMask = wSize - 1;
> - head = ArrayFill.fill(new int[HASH_SIZE], NO_MATCH);
> + head = new int[HASH_SIZE];
> + Arrays.fill(head, NO_MATCH);
> prev = new int[wSize];
> }
>
> diff --git
> a/src/main/java/org/apache/commons/compress/harmony/unpack200/Archive.java
> b/src/main/java/org/apache/commons/compress/harmony/unpack200/Archive.java
> index 6723e4a16..df957d780 100644
> ---
> a/src/main/java/org/apache/commons/compress/harmony/unpack200/Archive.java
> +++
> b/src/main/java/org/apache/commons/compress/harmony/unpack200/Archive.java
> @@ -78,7 +78,7 @@ public class Archive {
> this.inputStream =
> Pack200UnpackerAdapter.newBoundedInputStream(inputStream);
> this.outputStream = outputStream;
> if (inputStream instanceof FileInputStream) {
> - inputPath =
> Paths.get(Pack200UnpackerAdapter.readPathString((FileInputStream)
> inputStream));
> + inputPath = Pack200UnpackerAdapter.readPath((FileInputStream)
> inputStream);
> } else {
> inputPath = null;
> }
> diff --git
> a/src/main/java/org/apache/commons/compress/harmony/unpack200/Pack200UnpackerAdapter.java
> b/src/main/java/org/apache/commons/compress/harmony/unpack200/Pack200UnpackerAdapter.java
> index 83f5c7367..30cadea82 100644
> ---
> a/src/main/java/org/apache/commons/compress/harmony/unpack200/Pack200UnpackerAdapter.java
> +++
> b/src/main/java/org/apache/commons/compress/harmony/unpack200/Pack200UnpackerAdapter.java
> @@ -22,6 +22,7 @@ import java.io.FileInputStream;
> import java.io.FilterInputStream;
> import java.io.IOException;
> import java.io.InputStream;
> +import java.lang.reflect.Field;
> import java.net.URISyntaxException;
> import java.net.URL;
> import java.nio.file.Files;
> @@ -34,7 +35,6 @@ import
> org.apache.commons.compress.harmony.pack200.Pack200Exception;
> import org.apache.commons.compress.java.util.jar.Pack200.Unpacker;
> import org.apache.commons.io.input.BoundedInputStream;
> import org.apache.commons.io.input.CloseShieldInputStream;
> -import org.apache.commons.lang3.reflect.FieldUtils;
>
> /**
> * This class provides the binding between the standard Pack200 interface
> and the internal interface for (un)packing.
> @@ -56,7 +56,7 @@ public class Pack200UnpackerAdapter extends
> Pack200Adapter implements Unpacker {
> }
>
> private static BoundedInputStream newBoundedInputStream(final
> FileInputStream fileInputStream) throws IOException {
> - return newBoundedInputStream(readPathString(fileInputStream));
> + return newBoundedInputStream(readPath(fileInputStream));
> }
>
> @SuppressWarnings("resource") // Caller closes.
> @@ -130,19 +130,16 @@ public class Pack200UnpackerAdapter extends
> Pack200Adapter implements Unpacker {
> return newBoundedInputStream(Paths.get(url.toURI()));
> }
>
> - @SuppressWarnings("unchecked")
> - private static <T> T readField(final Object object, final String
> fieldName) {
> + static Path readPath(final FileInputStream fis) {
> try {
> - return (T) FieldUtils.readField(object, fieldName, true);
> - } catch (final IllegalAccessException e) {
> - return null;
> + Field field = FileInputStream.class.getDeclaredField("path");
> + field.setAccessible(true);
> + return Paths.get((String) field.get(fis));
> + } catch (NoSuchFieldException | IllegalAccessException e) {
> + throw new RuntimeException("Failed to read the path from the
> FileInputStream", e);
> }
> }
>
> - static String readPathString(final FileInputStream fis) {
> - return readField(fis, "path");
> - }
> -
> /**
> * Unwraps the given FilterInputStream to return its wrapped
> InputStream.
> *
> @@ -150,7 +147,13 @@ public class Pack200UnpackerAdapter extends
> Pack200Adapter implements Unpacker {
> * @return The wrapped InputStream
> */
> static InputStream unwrap(final FilterInputStream filterInputStream) {
> - return readField(filterInputStream, "in");
> + try {
> + Field field = FilterInputStream.class.getDeclaredField("in");
> + field.setAccessible(true);
> + return (InputStream) field.get(filterInputStream);
> + } catch (NoSuchFieldException | IllegalAccessException e) {
> + throw new RuntimeException("Failed to unwrap the
> FilterInputStream", e);
> + }
> }
>
> /**
> diff --git
> a/src/test/java/org/apache/commons/compress/archivers/tar/TarArchiveOutputStreamTest.java
> b/src/test/java/org/apache/commons/compress/archivers/tar/TarArchiveOutputStreamTest.java
> index e1fd352a1..cbfee27e2 100644
> ---
> a/src/test/java/org/apache/commons/compress/archivers/tar/TarArchiveOutputStreamTest.java
> +++
> b/src/test/java/org/apache/commons/compress/archivers/tar/TarArchiveOutputStreamTest.java
> @@ -48,7 +48,6 @@ import
> org.apache.commons.compress.archivers.ArchiveOutputStream;
> import org.apache.commons.compress.archivers.ArchiveStreamFactory;
> import org.apache.commons.io.IOUtils;
> import org.apache.commons.io.output.NullOutputStream;
> -import org.apache.commons.lang3.ArrayFill;
> import org.junit.jupiter.api.Disabled;
> import org.junit.jupiter.api.Test;
>
> @@ -148,7 +147,8 @@ public class TarArchiveOutputStreamTest extends
> AbstractTest {
> assertThrows(IllegalArgumentException.class, () -> testPadding(0,
> fileName, contents));
>
> // test with "content" that is an exact multiple of record length
> - final byte[] contents2 = ArrayFill.fill(new byte[2048], (byte)
> 42);
> + final byte[] contents2 = new byte[2048];
> + java.util.Arrays.fill(contents2, (byte) 42);
> testPadding(TarConstants.DEFAULT_BLKSIZE, fileName, contents2);
> }
>
> diff --git
> a/src/test/java/org/apache/commons/compress/archivers/zip/ZipArchiveInputStreamTest.java
> b/src/test/java/org/apache/commons/compress/archivers/zip/ZipArchiveInputStreamTest.java
> index 0a2fd6506..50551d300 100644
> ---
> a/src/test/java/org/apache/commons/compress/archivers/zip/ZipArchiveInputStreamTest.java
> +++
> b/src/test/java/org/apache/commons/compress/archivers/zip/ZipArchiveInputStreamTest.java
> @@ -40,6 +40,7 @@ import java.nio.file.Files;
> import java.nio.file.Path;
> import java.nio.file.Paths;
> import java.time.Instant;
> +import java.util.Arrays;
> import java.util.zip.ZipEntry;
> import java.util.zip.ZipException;
>
> @@ -49,7 +50,6 @@ import
> org.apache.commons.compress.archivers.ArchiveInputStream;
> import org.apache.commons.compress.archivers.ArchiveStreamFactory;
> import org.apache.commons.compress.utils.ByteUtils;
> import org.apache.commons.io.IOUtils;
> -import org.apache.commons.lang3.ArrayFill;
> import org.junit.jupiter.api.Test;
> import org.junit.jupiter.params.ParameterizedTest;
> import org.junit.jupiter.params.provider.ValueSource;
> @@ -666,7 +666,8 @@ public class ZipArchiveInputStreamTest extends
> AbstractTest {
> try (ZipArchiveInputStream in = new
> ZipArchiveInputStream(newInputStream("bzip2-zip.zip"))) {
> final ZipArchiveEntry ze = in.getNextZipEntry();
> assertEquals(42, ze.getSize());
> - final byte[] expected = ArrayFill.fill(new byte[42], (byte)
> 'a');
> + final byte[] expected = new byte[42];
> + Arrays.fill(expected, (byte) 'a');
> assertArrayEquals(expected, IOUtils.toByteArray(in));
> }
> }
> diff --git
> a/src/test/java/org/apache/commons/compress/archivers/zip/ZipFileTest.java
> b/src/test/java/org/apache/commons/compress/archivers/zip/ZipFileTest.java
> index c2501d15d..967652187 100644
> ---
> a/src/test/java/org/apache/commons/compress/archivers/zip/ZipFileTest.java
> +++
> b/src/test/java/org/apache/commons/compress/archivers/zip/ZipFileTest.java
> @@ -58,7 +58,6 @@ import org.apache.commons.compress.utils.ByteUtils;
> import org.apache.commons.compress.utils.SeekableInMemoryByteChannel;
> import org.apache.commons.io.IOUtils;
> import org.apache.commons.io.function.IORunnable;
> -import org.apache.commons.lang3.ArrayFill;
> import org.apache.commons.lang3.SystemUtils;
> import org.junit.Assume;
> import org.junit.jupiter.api.AfterEach;
> @@ -944,7 +943,8 @@ public class ZipFileTest extends AbstractTest {
> zf = new ZipFile(archive);
> final ZipArchiveEntry ze = zf.getEntry("lots-of-as");
> assertEquals(42, ze.getSize());
> - final byte[] expected = ArrayFill.fill(new byte[42], (byte) 'a');
> + final byte[] expected = new byte[42];
> + Arrays.fill(expected, (byte) 'a');
> try (InputStream inputStream = zf.getInputStream(ze)) {
> assertArrayEquals(expected, IOUtils.toByteArray(inputStream));
> }
> diff --git
> a/src/test/java/org/apache/commons/compress/compressors/lz4/BlockLZ4CompressorOutputStreamTest.java
> b/src/test/java/org/apache/commons/compress/compressors/lz4/BlockLZ4CompressorOutputStreamTest.java
> index 4f4dfb571..6d545472c 100644
> ---
> a/src/test/java/org/apache/commons/compress/compressors/lz4/BlockLZ4CompressorOutputStreamTest.java
> +++
> b/src/test/java/org/apache/commons/compress/compressors/lz4/BlockLZ4CompressorOutputStreamTest.java
> @@ -28,7 +28,6 @@ import java.io.IOException;
> import java.util.Arrays;
>
> import org.apache.commons.compress.compressors.lz77support.LZ77Compressor;
> -import org.apache.commons.lang3.ArrayFill;
> import org.junit.jupiter.api.Disabled;
> import org.junit.jupiter.api.Test;
>
> @@ -75,7 +74,9 @@ public class BlockLZ4CompressorOutputStreamTest {
> }
>
> private byte[] prepareExpected(final int length) {
> - return ArrayFill.fill(new byte[length], (byte) -1);
> + final byte[] b = new byte[length];
> + Arrays.fill(b, (byte) -1);
> + return b;
> }
>
> @Test
>
>
Re: (commons-compress) branch master updated: Removed the runtime dependency on commons-lang3
Posted by sebb <se...@gmail.com>.
It looks to me like there are several different changes here.
It seems to me that using standard library methods for ArrayFill and
SystemProperties is a sensible change.
I'm not sure about the other change as it does affect the behaviour,
and does look more complicated.
Perhaps it could be recast?
On Thu, 24 Oct 2024 at 20:05, Gary Gregory <ga...@gmail.com> wrote:
>
> Hello Emmanuel,
>
> Uh, no, don't be disingenuous by showing 3 lines, the commit is many pages
> long of diff. All of it is -1 as it increases the code complexity, increase
> code duplication, and adds the anti-pattern of throwing RuntimeException
> more than once.
>
> Please revert.
>
> Gary
>
> On Thu, Oct 24, 2024, 2:05 PM Emmanuel Bourg <eb...@apache.org> wrote:
>
> > Hi Gary,
> >
> > Le 24/10/2024 à 19:25, Gary Gregory a écrit :
> >
> > > No copypasta, please. I want to reduce code patterns that are duplicated
> > > all over the place.
> >
> > What do you mean by "copypasta"? Are you concerned about the maintenance
> > cost of the 3 lines of reflection code appearing twice in
> > Pack200UnpackerAdapter?
> >
> > Field field = FilterInputStream.class.getDeclaredField("in");
> > field.setAccessible(true);
> > return (InputStream) field.get(filterInputStream);
> >
> > Emmanuel Bourg
> >
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> > For additional commands, e-mail: dev-help@commons.apache.org
> >
> >
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: (commons-compress) branch master updated: Removed the runtime dependency on commons-lang3
Posted by Emmanuel Bourg <eb...@apache.org>.
Le 17/11/2024 à 23:57, sebb a écrit :
> It seems to me perfectly OK to question a commit regardless of when it happened.
>
> I think Emmanuel is within his rights to question whether adding the
> dependency was the right thing to do.
And I think I'm not the only one surprised by the new dependencies. I
stumbled on this commit from Torsten Curdt, another Commons Compress
contributor, in its jdeb project. The comment in the pom.xml is quite clear:
https://github.com/tcurdt/jdeb/commit/743e5d7f
From 743e5d7f387098c1d1190189f7636045a0fe004a Mon Sep 17 00:00:00 2001
From: Torsten Curdt <tc...@vafer.org>
Date: Sun, 2 Jun 2024 21:30:34 +0200
Subject: [PATCH] working around a dep issue with compress
---
pom.xml | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/pom.xml b/pom.xml
index 671f4bfce..7ea500b60 100644
--- a/pom.xml
+++ b/pom.xml
@@ -146,6 +146,12 @@
<artifactId>commons-compress</artifactId>
<version>1.26.2</version>
</dependency>
+ <dependency>
+ <!-- this should not be required but somehow compress introduced
the dep -->
+ <groupId>org.apache.commons</groupId>
+ <artifactId>commons-lang3</artifactId>
+ <version>3.14.0</version>
+ </dependency>
<dependency>
<groupId>org.bouncycastle</groupId>
<artifactId>bcpg-jdk15on</artifactId>
@@ -357,6 +363,7 @@
<include>org.apache.commons:commons-compress</include>
<include>commons-codec:commons-codec</include>
<include>commons-io:commons-io</include>
+ <include>org.apache.commons:commons-lang3</include>
<include>org.bouncycastle:bcpg-jdk15on</include>
<include>org.bouncycastle:bcprov-jdk15on</include>
<include>org.tukaani:xz</include>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: (commons-compress) branch master updated: Removed the runtime dependency on commons-lang3
Posted by sebb <se...@gmail.com>.
On Sun, 17 Nov 2024 at 14:01, Gary Gregory <ga...@gmail.com> wrote:
>
> Hi Niall,
>
> I don't think we are at an impasse: The PMC has approved release after
> release, as recently as 1.27.1 three months ago. Emmanuel has not voted on
> a Compress release in over eight years, since 1.11, that's 20 release votes
> ignored.
So what?
People come and go in Commons, and may change focus.
> Doing drive-by copypasta commits long after releases wanting a time machine
> is not OK IMO.
It seems to me perfectly OK to question a commit regardless of when it happened.
I think Emmanuel is within his rights to question whether adding the
dependency was the right thing to do.
> Gary
>
> On Sat, Nov 16, 2024, 3:42 PM Niall Pemberton <ni...@gmail.com>
> wrote:
>
> > On Fri, 1 Nov 2024 at 13:22, Gary D. Gregory <gg...@apache.org> wrote:
> >
> > > There has been no revert from the committer since the -1 on October 24,
> > so
> > > I will revert. This affects 11 Java files. This will revert the copy and
> > > pasting of code patterns over and over.
> >
> >
> > Seems we’re now at an impasses as Emmanuel has vetoed the commits
> > introducing the dependencies earlier in the year. So are you now going to
> > revert those?
> >
> > Niall
> >
> >
> >
> >
> > >
> > > Gary
> > >
> > > On 2024/10/24 21:17:41 Emmanuel Bourg wrote:
> > > > Le 24/10/2024 à 21:05, Gary Gregory a écrit :
> > > >
> > > > > Uh, no, don't be disingenuous by showing 3 lines, the commit is many
> > > pages
> > > > > long of diff. All of it is -1 as it increases the code complexity,
> > > increase
> > > > > code duplication, and adds the anti-pattern of throwing
> > > RuntimeException
> > > > > more than once.
> > > >
> > > > My commit message could have been clearer, it actually reverted the
> > > > commit 87e898fa [1] and replaced the reflection code with the snippet I
> > > > mentioned. So the code doesn't come out of my hat, that's what
> > > > commons-compress already had for years before.
> > > >
> > > > Adding a dependency such as commons-lang3 has a significant impact for
> > > > the users. In my case it broke an application that was using
> > > > commons-compress but didn't have commons-lang3 on the classpath.
> > > >
> > > > I understand your desire to reuse as much code as possible to ease the
> > > > maintenance, but in this case the user's convenience outweighs the
> > > > maintainer's convenience. commons-lang3 doesn't bring enough value in
> > > > this case to justify its addition to the runtime dependencies.
> > > >
> > > > Emmanuel Bourg
> > > >
> > > > [1] https://github.com/apache/commons-compress/commit/87e898fa
> > > >
> > > >
> > > > ---------------------------------------------------------------------
> > > > To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> > > > For additional commands, e-mail: dev-help@commons.apache.org
> > > >
> > > >
> > >
> > > ---------------------------------------------------------------------
> > > To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> > > For additional commands, e-mail: dev-help@commons.apache.org
> > >
> > >
> >
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: (commons-compress) branch master updated: Removed the runtime dependency on commons-lang3
Posted by Gary Gregory <ga...@gmail.com>.
Hi Niall,
I don't think we are at an impasse: The PMC has approved release after
release, as recently as 1.27.1 three months ago. Emmanuel has not voted on
a Compress release in over eight years, since 1.11, that's 20 release votes
ignored.
Doing drive-by copypasta commits long after releases wanting a time machine
is not OK IMO.
Gary
On Sat, Nov 16, 2024, 3:42 PM Niall Pemberton <ni...@gmail.com>
wrote:
> On Fri, 1 Nov 2024 at 13:22, Gary D. Gregory <gg...@apache.org> wrote:
>
> > There has been no revert from the committer since the -1 on October 24,
> so
> > I will revert. This affects 11 Java files. This will revert the copy and
> > pasting of code patterns over and over.
>
>
> Seems we’re now at an impasses as Emmanuel has vetoed the commits
> introducing the dependencies earlier in the year. So are you now going to
> revert those?
>
> Niall
>
>
>
>
> >
> > Gary
> >
> > On 2024/10/24 21:17:41 Emmanuel Bourg wrote:
> > > Le 24/10/2024 à 21:05, Gary Gregory a écrit :
> > >
> > > > Uh, no, don't be disingenuous by showing 3 lines, the commit is many
> > pages
> > > > long of diff. All of it is -1 as it increases the code complexity,
> > increase
> > > > code duplication, and adds the anti-pattern of throwing
> > RuntimeException
> > > > more than once.
> > >
> > > My commit message could have been clearer, it actually reverted the
> > > commit 87e898fa [1] and replaced the reflection code with the snippet I
> > > mentioned. So the code doesn't come out of my hat, that's what
> > > commons-compress already had for years before.
> > >
> > > Adding a dependency such as commons-lang3 has a significant impact for
> > > the users. In my case it broke an application that was using
> > > commons-compress but didn't have commons-lang3 on the classpath.
> > >
> > > I understand your desire to reuse as much code as possible to ease the
> > > maintenance, but in this case the user's convenience outweighs the
> > > maintainer's convenience. commons-lang3 doesn't bring enough value in
> > > this case to justify its addition to the runtime dependencies.
> > >
> > > Emmanuel Bourg
> > >
> > > [1] https://github.com/apache/commons-compress/commit/87e898fa
> > >
> > >
> > > ---------------------------------------------------------------------
> > > To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> > > For additional commands, e-mail: dev-help@commons.apache.org
> > >
> > >
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> > For additional commands, e-mail: dev-help@commons.apache.org
> >
> >
>
Re: (commons-compress) branch master updated: Removed the runtime dependency on commons-lang3
Posted by Niall Pemberton <ni...@gmail.com>.
On Fri, 1 Nov 2024 at 13:22, Gary D. Gregory <gg...@apache.org> wrote:
> There has been no revert from the committer since the -1 on October 24, so
> I will revert. This affects 11 Java files. This will revert the copy and
> pasting of code patterns over and over.
Seems we’re now at an impasses as Emmanuel has vetoed the commits
introducing the dependencies earlier in the year. So are you now going to
revert those?
Niall
>
> Gary
>
> On 2024/10/24 21:17:41 Emmanuel Bourg wrote:
> > Le 24/10/2024 à 21:05, Gary Gregory a écrit :
> >
> > > Uh, no, don't be disingenuous by showing 3 lines, the commit is many
> pages
> > > long of diff. All of it is -1 as it increases the code complexity,
> increase
> > > code duplication, and adds the anti-pattern of throwing
> RuntimeException
> > > more than once.
> >
> > My commit message could have been clearer, it actually reverted the
> > commit 87e898fa [1] and replaced the reflection code with the snippet I
> > mentioned. So the code doesn't come out of my hat, that's what
> > commons-compress already had for years before.
> >
> > Adding a dependency such as commons-lang3 has a significant impact for
> > the users. In my case it broke an application that was using
> > commons-compress but didn't have commons-lang3 on the classpath.
> >
> > I understand your desire to reuse as much code as possible to ease the
> > maintenance, but in this case the user's convenience outweighs the
> > maintainer's convenience. commons-lang3 doesn't bring enough value in
> > this case to justify its addition to the runtime dependencies.
> >
> > Emmanuel Bourg
> >
> > [1] https://github.com/apache/commons-compress/commit/87e898fa
> >
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> > For additional commands, e-mail: dev-help@commons.apache.org
> >
> >
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
>
Re: (commons-compress) branch master updated: Removed the runtime dependency on commons-lang3
Posted by Richard Zowalla <ri...@zowalla.com>.
From the sideline: I am also concerned about adding dependendcies in low level libraries for replacing only a few lines of code.
From ASF policy perspective, the veto (-1) requires a technical explanation, cf. https://www.apache.org/foundation/voting.html#Veto
"To prevent vetoes from being used capriciously, the voter must provide with the veto a technical justification showing why the change is bad (opens a security exposure, negatively affects performance, etc. ). A veto without a justification is invalid and has no weight."
So IMHO asking for an explanation about the actual CnP is totally fine.
Gruß
Richard
Am 1. November 2024 15:22:07 MEZ schrieb Emmanuel Bourg <eb...@apache.org>:
>Hi Gary,
>
>Le 01/11/2024 à 14:22, Gary D. Gregory a écrit :
>> There has been no revert from the committer since the -1 on October 24, so I will revert. This affects 11 Java files. This will revert the copy and pasting of code patterns over and over.
>
>I don't understand your "copy pasting" argument, the commit replaced exactly 8 occurrences of:
>
> int[] array = ArrayFill.fill(new int[length], value);
>
>with:
>
> int[] array = new int[length];
> Arrays.fill(array, value);
>
>
>It also replaced:
>
> SystemProperties.getOsName();
>
>with:
>
> System.getProperty("os.name");
>
>
>And two reflexive calls such as:
>
> FieldUtils.readField(object, fieldName, true);
>
>with:
>
> Field field = InputStream.class.getDeclaredField(fieldName);
> field.setAccessible(true);
> return field.get(object);
>
>
>These are not unreasonable substitutions to avoid a 660KB dependency. You make it sound like I copy-pasted tons of lines, but that's only 6 more lines of code overall.
>
>
>Let me put this differently, I'm -1 to your commits bf50b7d9 [1], 87e898fa [2] and 09031c91 [3] from earlier this year that introduced the dependency on Commons Lang in the first place. I'm especially unhappy with the first one, a mix of unrelated changes confusingly labeled as "Internal refactoring", where the dependency sneaked into without notice, neither mentioned in the commit message nor in the changelog. This dependency is an unnecessary bloat.
>
>I'm also concerned about the extra dependency on Commons Codec and Commons IO, also added this year. I'd like to return to the state of Commons Compress 1.25 with *zero* runtime dependency (besides the optional ones required for the various compression algorithms supported). For Codec and IO I suggest shading the classes as a compromise.
>
>Emmanuel Bourg
>
>[1] https://github.com/apache/commons-compress/commit/bf50b7d9
>[2] https://github.com/apache/commons-compress/commit/87e898fa
>[3] https://github.com/apache/commons-compress/commit/09031c91
>
>
>---------------------------------------------------------------------
>To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
>For additional commands, e-mail: dev-help@commons.apache.org
>
Re: (commons-compress) branch master updated: Removed the runtime dependency on commons-lang3
Posted by sebb <se...@gmail.com>.
On Fri, 1 Nov 2024 at 14:22, Emmanuel Bourg <eb...@apache.org> wrote:
>
> Hi Gary,
>
> Le 01/11/2024 à 14:22, Gary D. Gregory a écrit :
> > There has been no revert from the committer since the -1 on October 24, so I will revert. This affects 11 Java files. This will revert the copy and pasting of code patterns over and over.
>
> I don't understand your "copy pasting" argument, the commit replaced
> exactly 8 occurrences of:
>
> int[] array = ArrayFill.fill(new int[length], value);
>
> with:
>
> int[] array = new int[length];
> Arrays.fill(array, value);
>
>
> It also replaced:
>
> SystemProperties.getOsName();
>
> with:
>
> System.getProperty("os.name");
>
>
> And two reflexive calls such as:
>
> FieldUtils.readField(object, fieldName, true);
>
> with:
>
> Field field = InputStream.class.getDeclaredField(fieldName);
> field.setAccessible(true);
> return field.get(object);
>
>
> These are not unreasonable substitutions to avoid a 660KB dependency.
> You make it sound like I copy-pasted tons of lines, but that's only 6
> more lines of code overall.
>
>
> Let me put this differently, I'm -1 to your commits bf50b7d9 [1],
> 87e898fa [2] and 09031c91 [3] from earlier this year that introduced the
> dependency on Commons Lang in the first place. I'm especially unhappy
> with the first one, a mix of unrelated changes confusingly labeled as
> "Internal refactoring", where the dependency sneaked into without
> notice, neither mentioned in the commit message nor in the changelog.
> This dependency is an unnecessary bloat.
+1
> I'm also concerned about the extra dependency on Commons Codec and
> Commons IO, also added this year. I'd like to return to the state of
> Commons Compress 1.25 with *zero* runtime dependency (besides the
> optional ones required for the various compression algorithms
> supported). For Codec and IO I suggest shading the classes as a compromise.
>
> Emmanuel Bourg
>
> [1] https://github.com/apache/commons-compress/commit/bf50b7d9
> [2] https://github.com/apache/commons-compress/commit/87e898fa
> [3] https://github.com/apache/commons-compress/commit/09031c91
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: (commons-compress) branch master updated: Removed the runtime dependency on commons-lang3
Posted by Emmanuel Bourg <eb...@apache.org>.
Hi Gary,
Le 01/11/2024 à 14:22, Gary D. Gregory a écrit :
> There has been no revert from the committer since the -1 on October 24, so I will revert. This affects 11 Java files. This will revert the copy and pasting of code patterns over and over.
I don't understand your "copy pasting" argument, the commit replaced
exactly 8 occurrences of:
int[] array = ArrayFill.fill(new int[length], value);
with:
int[] array = new int[length];
Arrays.fill(array, value);
It also replaced:
SystemProperties.getOsName();
with:
System.getProperty("os.name");
And two reflexive calls such as:
FieldUtils.readField(object, fieldName, true);
with:
Field field = InputStream.class.getDeclaredField(fieldName);
field.setAccessible(true);
return field.get(object);
These are not unreasonable substitutions to avoid a 660KB dependency.
You make it sound like I copy-pasted tons of lines, but that's only 6
more lines of code overall.
Let me put this differently, I'm -1 to your commits bf50b7d9 [1],
87e898fa [2] and 09031c91 [3] from earlier this year that introduced the
dependency on Commons Lang in the first place. I'm especially unhappy
with the first one, a mix of unrelated changes confusingly labeled as
"Internal refactoring", where the dependency sneaked into without
notice, neither mentioned in the commit message nor in the changelog.
This dependency is an unnecessary bloat.
I'm also concerned about the extra dependency on Commons Codec and
Commons IO, also added this year. I'd like to return to the state of
Commons Compress 1.25 with *zero* runtime dependency (besides the
optional ones required for the various compression algorithms
supported). For Codec and IO I suggest shading the classes as a compromise.
Emmanuel Bourg
[1] https://github.com/apache/commons-compress/commit/bf50b7d9
[2] https://github.com/apache/commons-compress/commit/87e898fa
[3] https://github.com/apache/commons-compress/commit/09031c91
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: (commons-compress) branch master updated: Removed the runtime dependency on commons-lang3
Posted by "Gary D. Gregory" <gg...@apache.org>.
There has been no revert from the committer since the -1 on October 24, so I will revert. This affects 11 Java files. This will revert the copy and pasting of code patterns over and over.
Gary
On 2024/10/24 21:17:41 Emmanuel Bourg wrote:
> Le 24/10/2024 à 21:05, Gary Gregory a écrit :
>
> > Uh, no, don't be disingenuous by showing 3 lines, the commit is many pages
> > long of diff. All of it is -1 as it increases the code complexity, increase
> > code duplication, and adds the anti-pattern of throwing RuntimeException
> > more than once.
>
> My commit message could have been clearer, it actually reverted the
> commit 87e898fa [1] and replaced the reflection code with the snippet I
> mentioned. So the code doesn't come out of my hat, that's what
> commons-compress already had for years before.
>
> Adding a dependency such as commons-lang3 has a significant impact for
> the users. In my case it broke an application that was using
> commons-compress but didn't have commons-lang3 on the classpath.
>
> I understand your desire to reuse as much code as possible to ease the
> maintenance, but in this case the user's convenience outweighs the
> maintainer's convenience. commons-lang3 doesn't bring enough value in
> this case to justify its addition to the runtime dependencies.
>
> Emmanuel Bourg
>
> [1] https://github.com/apache/commons-compress/commit/87e898fa
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: (commons-compress) branch master updated: Removed the runtime dependency on commons-lang3
Posted by Emmanuel Bourg <eb...@apache.org>.
Le 24/10/2024 à 21:05, Gary Gregory a écrit :
> Uh, no, don't be disingenuous by showing 3 lines, the commit is many pages
> long of diff. All of it is -1 as it increases the code complexity, increase
> code duplication, and adds the anti-pattern of throwing RuntimeException
> more than once.
My commit message could have been clearer, it actually reverted the
commit 87e898fa [1] and replaced the reflection code with the snippet I
mentioned. So the code doesn't come out of my hat, that's what
commons-compress already had for years before.
Adding a dependency such as commons-lang3 has a significant impact for
the users. In my case it broke an application that was using
commons-compress but didn't have commons-lang3 on the classpath.
I understand your desire to reuse as much code as possible to ease the
maintenance, but in this case the user's convenience outweighs the
maintainer's convenience. commons-lang3 doesn't bring enough value in
this case to justify its addition to the runtime dependencies.
Emmanuel Bourg
[1] https://github.com/apache/commons-compress/commit/87e898fa
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: (commons-compress) branch master updated: Removed the runtime dependency on commons-lang3
Posted by Gary Gregory <ga...@gmail.com>.
Hello Emmanuel,
Uh, no, don't be disingenuous by showing 3 lines, the commit is many pages
long of diff. All of it is -1 as it increases the code complexity, increase
code duplication, and adds the anti-pattern of throwing RuntimeException
more than once.
Please revert.
Gary
On Thu, Oct 24, 2024, 2:05 PM Emmanuel Bourg <eb...@apache.org> wrote:
> Hi Gary,
>
> Le 24/10/2024 à 19:25, Gary Gregory a écrit :
>
> > No copypasta, please. I want to reduce code patterns that are duplicated
> > all over the place.
>
> What do you mean by "copypasta"? Are you concerned about the maintenance
> cost of the 3 lines of reflection code appearing twice in
> Pack200UnpackerAdapter?
>
> Field field = FilterInputStream.class.getDeclaredField("in");
> field.setAccessible(true);
> return (InputStream) field.get(filterInputStream);
>
> Emmanuel Bourg
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
>
Re: (commons-compress) branch master updated: Removed the runtime dependency on commons-lang3
Posted by Emmanuel Bourg <eb...@apache.org>.
Hi Gary,
Le 24/10/2024 à 19:25, Gary Gregory a écrit :
> No copypasta, please. I want to reduce code patterns that are duplicated
> all over the place.
What do you mean by "copypasta"? Are you concerned about the maintenance
cost of the 3 lines of reflection code appearing twice in
Pack200UnpackerAdapter?
Field field = FilterInputStream.class.getDeclaredField("in");
field.setAccessible(true);
return (InputStream) field.get(filterInputStream);
Emmanuel Bourg
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org