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 2024/11/17 16:51:50 UTC
[PR] Remove Commons Lang dependency [commons-compress]
ppkarwasz opened a new pull request, #607:
URL: https://github.com/apache/commons-compress/pull/607
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.
--
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] Remove Commons Lang dependency [commons-compress]
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on code in PR #607:
URL: https://github.com/apache/commons-compress/pull/607#discussion_r1846326165
##########
src/main/java/org/apache/commons/compress/internal/lang3/SystemProperties.java:
##########
@@ -0,0 +1,33 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.commons.compress.internal.lang3;
+
+public final class SystemProperties {
+
+ public static String getOsName() {
+ try {
+ return System.getProperty("os.name");
Review Comment:
Fixed in https://github.com/apache/commons-compress/pull/607/commits/18a4442972842ce8e456d49e0589acbe4bd14aef
--
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] Remove Commons Lang dependency [commons-compress]
Posted by "vlsi (via GitHub)" <gi...@apache.org>.
vlsi commented on code in PR #607:
URL: https://github.com/apache/commons-compress/pull/607#discussion_r2485993145
##########
src/main/java/org/apache/commons/compress/internal/lang3/reflect/FieldUtils.java:
##########
@@ -0,0 +1,61 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.commons.compress.internal.lang3.reflect;
+
+import static java.util.Objects.requireNonNull;
+
+import java.lang.reflect.Field;
+
+public final class FieldUtils {
Review Comment:
This is used in a pack200 which has been deprecated for removal in Java 11.
Have you considered moving the implementation to pack200 so the other compressors won't accidentally depend on reflection-like approaches?
--
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] Remove Commons Lang dependency [commons-compress]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory closed pull request #607: Remove Commons Lang dependency
URL: https://github.com/apache/commons-compress/pull/607
--
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] Remove Commons Lang dependency [commons-compress]
Posted by "acasademont (via GitHub)" <gi...@apache.org>.
acasademont commented on PR #607:
URL: https://github.com/apache/commons-compress/pull/607#issuecomment-2751643052
We would really appreciate this one, it came as a surprise to use that when we updated from 1.25.0 we had to add a new dependency on commons lang.
--
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] Remove Commons Lang dependency [commons-compress]
Posted by "Glavo (via GitHub)" <gi...@apache.org>.
Glavo commented on PR #607:
URL: https://github.com/apache/commons-compress/pull/607#issuecomment-2514390847
I released [Kala Compress](https://github.com/Glavo/kala-compress) 1.27.1-2. It is based on commons-compress, but does not depend on commons-io and commons-lang. Maybe this will be what you want.
--
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] Remove Commons Lang dependency [commons-compress]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on PR #607:
URL: https://github.com/apache/commons-compress/pull/607#issuecomment-2482896197
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.
--
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] Remove Commons Lang dependency [commons-compress]
Posted by "ebourg (via GitHub)" <gi...@apache.org>.
ebourg commented on PR #607:
URL: https://github.com/apache/commons-compress/pull/607#issuecomment-4947538388
@garydgregory I still strongly disagree with your decision, there was no consensus for this change.
--
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] Remove Commons Lang dependency [commons-compress]
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on PR #607:
URL: https://github.com/apache/commons-compress/pull/607#issuecomment-2516939299
> I released [Kala Compress](https://github.com/Glavo/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](https://github.com/apache/commons-compress/tree/2.x). 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.
--
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] Remove Commons Lang dependency [commons-compress]
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on PR #607:
URL: https://github.com/apache/commons-compress/pull/607#issuecomment-2483078459
> 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:
```java
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:
```java
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()](https://docs.oracle.com/javase/8/docs/api/java/lang/System.html#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`](https://docs.oracle.com/javase/8/docs/api/java/io/FilterInputStream.html#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.
--
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] Remove Commons Lang dependency [commons-compress]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on PR #607:
URL: https://github.com/apache/commons-compress/pull/607#issuecomment-4946134972
Closing: Won't do.
--
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] Remove Commons Lang dependency [commons-compress]
Posted by "ebourg (via GitHub)" <gi...@apache.org>.
ebourg commented on code in PR #607:
URL: https://github.com/apache/commons-compress/pull/607#discussion_r1846104681
##########
src/main/java/org/apache/commons/compress/internal/lang3/SystemProperties.java:
##########
@@ -0,0 +1,33 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.commons.compress.internal.lang3;
+
+public final class SystemProperties {
+
+ public static String getOsName() {
+ try {
+ return System.getProperty("os.name");
Review Comment:
I wouldn't bother dealing with the SecurityManager, the `os.name` property is granted read access in the default policy anyway.
--
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] Remove Commons Lang dependency [commons-compress]
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on code in PR #607:
URL: https://github.com/apache/commons-compress/pull/607#discussion_r1846011458
##########
src/main/java/org/apache/commons/compress/internal/lang3/SystemProperties.java:
##########
@@ -0,0 +1,33 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.commons.compress.internal.lang3;
+
+public final class SystemProperties {
+
+ public static String getOsName() {
+ try {
+ return System.getProperty("os.name");
Review Comment:
The baseline of Commons Compress is Java 8, shouldn't we support `SecurityManager`?
--
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] Remove Commons Lang dependency [commons-compress]
Posted by "ebourg (via GitHub)" <gi...@apache.org>.
ebourg commented on code in PR #607:
URL: https://github.com/apache/commons-compress/pull/607#discussion_r1845656345
##########
src/main/java/org/apache/commons/compress/internal/lang3/SystemProperties.java:
##########
@@ -0,0 +1,33 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.commons.compress.internal.lang3;
+
+public final class SystemProperties {
+
+ public static String getOsName() {
+ try {
+ return System.getProperty("os.name");
Review Comment:
A direct call to System.getProperty("os.name") would be simpler
--
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