You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@commons.apache.org by "ppkarwasz (via GitHub)" <gi...@apache.org> on 2025/09/23 11:41:26 UTC
[PR] feat: introduce `ArchiveFile` abstraction for file-based archives [commons-compress]
ppkarwasz opened a new pull request, #709:
URL: https://github.com/apache/commons-compress/pull/709
This PR adds a new `ArchiveFile` interface to unify the handling of file-based archive utilities such as `SevenZFile`, `TarFile`, and `ZipFile`.
Although these classes target different archive formats, they share several core characteristics:
* All are `Closeable`.
* Each provides the same method to open an `InputStream` for a given entry (`InputStream getInputStream(T)` where `T extends ArchiveEntry`).
* Historically, their `getEntries()` methods returned incompatible types. This PR introduces a common `List<? extends T> entries()` method, aligning with `java.util.zip.ZipFile` in name but offering a modern return value.
* The `ZipFile#stream()` method (added in 1.28.0) is now part of this abstraction.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: introduce `ArchiveFile` abstraction for file-based archives [commons-compress]
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on PR #709:
URL: https://github.com/apache/commons-compress/pull/709#issuecomment-3323716407
In https://github.com/apache/commons-compress/pull/709/commits/3234bdb8a1fcb2722a839204e8d59731efbf967d I added the `IOIterable` interface that has been recently implemented in `TarFile`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: introduce `ArchiveFile` abstraction for file-based archives [commons-compress]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #709:
URL: https://github.com/apache/commons-compress/pull/709#discussion_r2372338640
##########
src/test/java/org/apache/commons/compress/archivers/tar/TarFileTest.java:
##########
@@ -40,15 +40,14 @@
import java.util.List;
import java.util.zip.GZIPInputStream;
-import org.apache.commons.compress.AbstractTest;
+import org.apache.commons.compress.archivers.AbstractArchiveFileTest;
import org.apache.commons.compress.archivers.ArchiveException;
import org.apache.commons.io.IOUtils;
+import org.apache.commons.lang3.StringUtils;
Review Comment:
The import has been changed from `shaded.org.apache.commons.lang3.StringUtils` to `org.apache.commons.lang3.StringUtils`. Ensure this change is intentional and that the non-shaded version provides the same functionality expected by the test code.
##########
src/main/java/org/apache/commons/compress/archivers/tar/TarFile.java:
##########
@@ -240,7 +238,11 @@ private TarFile(final Builder builder) throws IOException {
this.recordBuffer = ByteBuffer.allocate(this.recordSize);
this.blockSize = builder.getBlockSize();
this.lenient = builder.isLenient();
- forEach(entries::add);
+ // Read all entries eagerly
+ TarArchiveEntry entry;
+ while ((entry = getNextTarEntry()) != null) {
+ entries.add(entry);
+ }
Review Comment:
The implementation has changed from lazy loading using `forEach(entries::add)` to eager loading with an explicit while loop. This change should be documented or explained as it affects memory usage and initialization performance for large archives.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: introduce `ArchiveFile` abstraction for file-based archives [commons-compress]
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on code in PR #709:
URL: https://github.com/apache/commons-compress/pull/709#discussion_r2372345977
##########
src/test/java/org/apache/commons/compress/archivers/tar/TarFileTest.java:
##########
@@ -40,15 +40,14 @@
import java.util.List;
import java.util.zip.GZIPInputStream;
-import org.apache.commons.compress.AbstractTest;
+import org.apache.commons.compress.archivers.AbstractArchiveFileTest;
import org.apache.commons.compress.archivers.ArchiveException;
import org.apache.commons.io.IOUtils;
+import org.apache.commons.lang3.StringUtils;
Review Comment:
The change is intentional, using the shaded version wasn't.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add `ArchiveFile` abstraction for file-based archives [commons-compress]
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on PR #709:
URL: https://github.com/apache/commons-compress/pull/709#issuecomment-3364722029
I will retarget this PR to `commons_io_2_21_0`, once #721 is ready.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add `ArchiveFile` abstraction for file-based archives [commons-compress]
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on PR #709:
URL: https://github.com/apache/commons-compress/pull/709#issuecomment-3373790748
I retargetted this onto `commons_io_2_21_0`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: introduce `ArchiveFile` abstraction for file-based archives [commons-compress]
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on code in PR #709:
URL: https://github.com/apache/commons-compress/pull/709#discussion_r2373160073
##########
src/main/java/org/apache/commons/compress/archivers/tar/TarFile.java:
##########
@@ -240,7 +238,11 @@ private TarFile(final Builder builder) throws IOException {
this.recordBuffer = ByteBuffer.allocate(this.recordSize);
this.blockSize = builder.getBlockSize();
this.lenient = builder.isLenient();
- forEach(entries::add);
+ // Read all entries eagerly
+ TarArchiveEntry entry;
+ while ((entry = getNextTarEntry()) != null) {
+ entries.add(entry);
+ }
Review Comment:
The previous implementation wasn't lazy either, although we should probably implement `stream` in a lazy way.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: introduce `ArchiveFile` abstraction for file-based archives [commons-compress]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on code in PR #709:
URL: https://github.com/apache/commons-compress/pull/709#discussion_r2379083554
##########
src/main/java/org/apache/commons/compress/archivers/tar/TarFile.java:
##########
@@ -240,7 +238,11 @@ private TarFile(final Builder builder) throws IOException {
this.recordBuffer = ByteBuffer.allocate(this.recordSize);
this.blockSize = builder.getBlockSize();
this.lenient = builder.isLenient();
- forEach(entries::add);
Review Comment:
This look like a merge mistake.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add `ArchiveFile` abstraction for file-based archives [commons-compress]
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on code in PR #709:
URL: https://github.com/apache/commons-compress/pull/709#discussion_r2380273884
##########
src/main/java/org/apache/commons/compress/archivers/tar/TarFile.java:
##########
@@ -240,7 +238,11 @@ private TarFile(final Builder builder) throws IOException {
this.recordBuffer = ByteBuffer.allocate(this.recordSize);
this.blockSize = builder.getBlockSize();
this.lenient = builder.isLenient();
- forEach(entries::add);
Review Comment:
Reverting to the old code here is deliberate.
When I dropped your custom `iterator()` implementation in favor of the default from `ArchiveFile`, it introduced a loop:
* `forEach` in `IOIterable` calls `iterator()`.
* The default `iterator()` calls `entries()`:
https://github.com/apache/commons-compress/blob/f5dd39c8bcb1abb43270c75167008ba0330c6818/src/main/java/org/apache/commons/compress/archivers/ArchiveFile.java#L82-L86
* `entries()` calls `stream()`:
https://github.com/apache/commons-compress/blob/f5dd39c8bcb1abb43270c75167008ba0330c6818/src/main/java/org/apache/commons/compress/archivers/ArchiveFile.java#L54-L58
* `stream()` iterates over the `entries` collection, the very collection we’re trying to populate:
https://github.com/apache/commons-compress/blob/f5dd39c8bcb1abb43270c75167008ba0330c6818/src/main/java/org/apache/commons/compress/archivers/tar/TarFile.java#L464-L467
So `forEach(entries::add)` ends up iterating an empty collection and adding nothing, effectively a no-op. Restoring the original `iterator()` avoids this self-referential loop.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add `ArchiveFile` abstraction for file-based archives [commons-compress]
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on code in PR #709:
URL: https://github.com/apache/commons-compress/pull/709#discussion_r2408253971
##########
src/main/java/org/apache/commons/compress/archivers/tar/TarFile.java:
##########
@@ -240,7 +238,11 @@ private TarFile(final Builder builder) throws IOException {
this.recordBuffer = ByteBuffer.allocate(this.recordSize);
this.blockSize = builder.getBlockSize();
this.lenient = builder.isLenient();
- forEach(entries::add);
Review Comment:
I added a comment in https://github.com/apache/commons-compress/pull/709/commits/9b16aeb26c3f20ccf18cc7f978e242360f9e70e9
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add `ArchiveFile` abstraction for file-based archives [commons-compress]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on PR #709:
URL: https://github.com/apache/commons-compress/pull/709#issuecomment-3393644503
PR merged.
TY @ppkarwasz !
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add `ArchiveFile` abstraction for file-based archives [commons-compress]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory merged PR #709:
URL: https://github.com/apache/commons-compress/pull/709
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org