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/25 10:57:21 UTC
Re: (commons-configuration) 05/05: Remove the dependency on commons-codec
-1, please revert and stop inlining Commons methods all over itself.
Thank you,
Gary
On Fri, Oct 25, 2024 at 6:06 AM <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-configuration.git
>
> commit 720b5a9aa06994fd435b5fe589fc28a826ead48a
> Author: Emmanuel Bourg <eb...@apache.org>
> AuthorDate: Fri Oct 25 11:26:10 2024 +0200
>
> Remove the dependency on commons-codec
> ---
> pom.xml | 8 --------
> src/changes/changes.xml | 2 +-
> 2 files changed, 1 insertion(+), 9 deletions(-)
>
> diff --git a/pom.xml b/pom.xml
> index 08cbeae0..fa3d18e3 100644
> --- a/pom.xml
> +++ b/pom.xml
> @@ -49,7 +49,6 @@
> <!-- Explicitly declare optional dependencies for the OSGi manifest.
> -->
> <commons.osgi.import>
> org.apache.commons.beanutils.*;resolution:=optional,
> - org.apache.commons.codec.*;resolution:=optional,
> org.apache.commons.jxpath.*;resolution:=optional,
> org.apache.xml.resolver.*;resolution:=optional,
> javax.servlet.*;resolution:=optional,
> @@ -141,13 +140,6 @@
> <optional>true</optional>
> </dependency>
>
> - <dependency>
> - <groupId>commons-codec</groupId>
> - <artifactId>commons-codec</artifactId>
> - <version>1.17.1</version>
> - <optional>true</optional>
> - </dependency>
> -
> <dependency>
> <groupId>org.apache.commons</groupId>
> <artifactId>commons-jexl</artifactId>
> diff --git a/src/changes/changes.xml b/src/changes/changes.xml
> index cdadae96..b6be9901 100644
> --- a/src/changes/changes.xml
> +++ b/src/changes/changes.xml
> @@ -33,10 +33,10 @@
> <!-- ADD -->
> <action type="add" dev="ggregory" due-to="Gary Gregory">Add
> PrefixedKeysIterator.toString() to package-private
> PrefixedKeysIterator.</action>
> <!-- UPDATE -->
> + <action type="update" dev="ebourg" due-to="Emmanuel Bourg">The
> dependency on commons-codec has been removed</action>
> <action type="update" dev="ggregory" due-to="Gary Gregory,
> Dependabot">Bump org.apache.commons:commons-parent from 70 to 78 #434,
> #453, #466, #470, #479, #486, #491, #499.</action>
> <action type="update" dev="ggregory" due-to="Gary Gregory,
> Dependabot">Bump commons-logging:commons-logging from 1.3.2 to 1.3.4 #442,
> #467.</action>
> <action type="update" dev="ggregory" due-to="Gary Gregory,
> Dependabot">Bump com.fasterxml.jackson.core:jackson-databind from 2.17.1 to
> 2.18.0 #447, #483.</action>
> - <action type="update" dev="ggregory" due-to="Gary Gregory,
> Dependabot">Bump commons-codec:commons-codec from 1.17.0 to 1.17.1
> #449.</action>
> <action type="update" dev="ggregory" due-to="Gary Gregory,
> Dependabot">Bump org.apache.commons:commons-lang3 from 3.14.0 to 3.17.0
> #450, #458, #471.</action>
> <action type="update" dev="ggregory" due-to="Gary Gregory,
> Dependabot">Bump org.hamcrest:hamcrest from 2.2 to 3.0 #454.</action>
> <action type="update" dev="ggregory" due-to="Gary Gregory,
> Dependabot">Bump slf4j.version from 2.0.13 to 2.0.16 #457, #462,
> #464.</action>
>
>
Re: (commons-configuration) 05/05: Remove the dependency on commons-codec
Posted by Gilles Sadowski <gi...@gmail.com>.
Hi.
Le lun. 28 oct. 2024 à 14:36, Bernd Eckenfels <ec...@zusammenkunft.net> a écrit :
>
> Hello,
>
> I don’t think a new shared library for HexFormat is a good idea here and removing codec with a simple code inline would be my preference as well. Do we really want to maintain a new lib
Yes, if it solves the problem (of incompatible views).
How about this library being "for internal use only" (meaning that it should
not ever contain any feature not directly needed by another "Commons"
component)?
> for code which hasn’t changed in ages as it has a trivial specification.
Does this imply that the code must be copied everywhere it's needed?
[Gary does not agree...]
>
> Maybe to add another datapoint, with Java17 you can use the HexFornat utility and remove that inlined helper.
Indeed, that's the expected outcome in the longer term.
[As Emannuel laid out (with s/Java17/Java8).]
Gilles
>>> [...]
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: (commons-configuration) 05/05: Remove the dependency on commons-codec
Posted by Bernd Eckenfels <ec...@zusammenkunft.net>.
Hello,
I don’t think a new shared library for HexFormat is a good idea here and removing codec with a simple code inline would be my preference as well. Do we really want to maintain a new lib for code which hasn’t changed in ages as it has a trivial specification.
Maybe to add another datapoint, with Java17 you can use the HexFornat utility and remove that inlined helper.
Gruß
Bernd
Gilles Sadowski wrote on 28. Oct 2024 14:30 (GMT +01:00):
> Hi.
>
> Le lun. 28 oct. 2024 à 13:38, Emmanuel Bourg <eb...@apache.org> a écrit
> :
>>
>> Le 26/10/2024 à 20:32, Elliotte Rusty Harold a écrit :
>>
>> > That's exactly backwards. Code changes have to be technically
>> > justified. The responsibility is on the proposer, not the vetoer, to
>> > demonstrate that the change os worthwhile. Code changes are not
>> > accepted by default unless there's good and valid reason for them. If
>> > you can't get even one committer to +1 a PR thgen it doesn't meet the
>> > threshold for merging.
>>
>> It goes both ways, you can't veto without a good reason either. "I don't
>> like it" is definitely not one.
>>
>> It looks like I have to explain in more details the change, so here it
>> is:
>>
>> When I implemented the plist configuration in 2005 [1] I needed a Base64
>> and an hex encoder/decoder. Base64 isn't exactly trivial to implement
>> and Commons Codec already has one, so I imported the dependency.
>>
>> The inflation of dependencies was seen as a concern, the plist
>> configuration initially used Commons Digester which was eventually
>> replaced by pure SAX code in 2007 [2], Commons Collections was also used
>> and got removed in 2014 [3]. Some dependencies were made optional to
>> reduce the amount of dependencies pulled [4], but this made an awkward
>> user experience where unsuspecting users would get a
>> ClassNotFoundException when trying to use the plist configurations.
>>
>> Java 8 was released in 2014 with a long awaited Base64 implementation,
>> and Commons Configuration moved to Java 8 in 2018. I've updated the code
>> last week to use java.util.Base64 instead of Commons Codec [5].
>
> Up to here, I don't think that anyone would dispute that it is an
> overall improvement (more code reuse, less maintenance).
>
>> At this
>> point Commons Codec was only retained for encoding/decoding hexadecimal
>> values. Commons Codec weighs 360KB and must be added manually to the
>> classpath, encoding/decoding hexadecimal is trivially handled by ~20
>> lines of code, so dropping the dependency was the obvious choice to me
>> and in the spirit of the previous dependency removals.
>
> The last part involves the trade-off between "code reuse" and
> "dependency".
> 20 lines is small, sure, but every "bloat" has always started small; then
> the
> same argument applied for every new small bit of code: Not worth adding a
> dependency).
>
> So this circles back to the proposal in the thread with "Subject:"
> Usage of (Commons) dependencies
> That is: _If_ we had a ("very low-level") module that provides the 20
> lines
> needed by [Configuration] and [Lang], they could both depend on it, with
> almost no overhead or additional risks: No undue dependency (Emmanuel's
> point), potential fixes would be implemented once (Gary's point).
>
> We could then "manage" this via JIRA:
> 1. Open a report stating the need for the "very low-level" module and its
> scope (i.e. the list of features, such as the "20 lines" referred to
> here).
> 2. Open a JIRA sub-task: Replace the "20 lines" once the module is
> released.
> 3. Add a comment around the "20 lines" with a reference to the sub-task.
>
> How does that sound?
>
> Regards,
> Gilles
>
>
>
>>
>> Emmanuel Bourg
>>
>> [1]
>> https://github.com/apache/commons-configuration/commit/02f66818#diff-a8d5e2a5cbeb500e59aca88e0613283c57d530c6df7630555d5a08bc5c3a660fR258
>> [2] https://github.com/apache/commons-configuration/commit/8b26e6e6
>> [3] https://github.com/apache/commons-configuration/commit/25490ca9
>> [4] https://github.com/apache/commons-configuration/commit/63776cc0
>> [5] https://github.com/apache/commons-configuration/commit/7b434dbd
>>
>>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
>
Gruß
Bernd
—
https://bernd.eckenfels.net
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: (commons-configuration) 05/05: Remove the dependency on commons-codec
Posted by Emmanuel Bourg <eb...@apache.org>.
Le 28/10/2024 à 14:30, Gilles Sadowski a écrit :
> The last part involves the trade-off between "code reuse" and "dependency".
> 20 lines is small, sure, but every "bloat" has always started small; then the
> same argument applied for every new small bit of code: Not worth adding a
> dependency).
>
> So this circles back to the proposal in the thread with "Subject:"
> Usage of (Commons) dependencies
> That is: _If_ we had a ("very low-level") module that provides the 20 lines
> needed by [Configuration] and [Lang], they could both depend on it, with
> almost no overhead or additional risks: No undue dependency (Emmanuel's
> point), potential fixes would be implemented once (Gary's point).
>
> We could then "manage" this via JIRA:
> 1. Open a report stating the need for the "very low-level" module and its
> scope (i.e. the list of features, such as the "20 lines" referred to here).
> 2. Open a JIRA sub-task: Replace the "20 lines" once the module is
> released.
> 3. Add a comment around the "20 lines" with a reference to the sub-task.
>
> How does that sound?
If commons-configuration was depending on commons-codec-hex I don't
think I would have removed the dependency before the switch to Java 17.
But is it a good idea to create such an artifact now, just for the needs
of commons-configuration, obsolete for any other usage, and that will be
replaced by Java 17 HexFormat within 2 or 3 years? I don't think so. I'm
not opposed to the idea but it would be weird in my opinion. I'd rather
shade the commons-codec Hex class.
Emmanuel Bourg
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: (commons-configuration) 05/05: Remove the dependency on commons-codec
Posted by Gilles Sadowski <gi...@gmail.com>.
Hi.
Le lun. 28 oct. 2024 à 13:38, Emmanuel Bourg <eb...@apache.org> a écrit :
>
> Le 26/10/2024 à 20:32, Elliotte Rusty Harold a écrit :
>
> > That's exactly backwards. Code changes have to be technically
> > justified. The responsibility is on the proposer, not the vetoer, to
> > demonstrate that the change os worthwhile. Code changes are not
> > accepted by default unless there's good and valid reason for them. If
> > you can't get even one committer to +1 a PR thgen it doesn't meet the
> > threshold for merging.
>
> It goes both ways, you can't veto without a good reason either. "I don't
> like it" is definitely not one.
>
> It looks like I have to explain in more details the change, so here it is:
>
> When I implemented the plist configuration in 2005 [1] I needed a Base64
> and an hex encoder/decoder. Base64 isn't exactly trivial to implement
> and Commons Codec already has one, so I imported the dependency.
>
> The inflation of dependencies was seen as a concern, the plist
> configuration initially used Commons Digester which was eventually
> replaced by pure SAX code in 2007 [2], Commons Collections was also used
> and got removed in 2014 [3]. Some dependencies were made optional to
> reduce the amount of dependencies pulled [4], but this made an awkward
> user experience where unsuspecting users would get a
> ClassNotFoundException when trying to use the plist configurations.
>
> Java 8 was released in 2014 with a long awaited Base64 implementation,
> and Commons Configuration moved to Java 8 in 2018. I've updated the code
> last week to use java.util.Base64 instead of Commons Codec [5].
Up to here, I don't think that anyone would dispute that it is an
overall improvement (more code reuse, less maintenance).
> At this
> point Commons Codec was only retained for encoding/decoding hexadecimal
> values. Commons Codec weighs 360KB and must be added manually to the
> classpath, encoding/decoding hexadecimal is trivially handled by ~20
> lines of code, so dropping the dependency was the obvious choice to me
> and in the spirit of the previous dependency removals.
The last part involves the trade-off between "code reuse" and "dependency".
20 lines is small, sure, but every "bloat" has always started small; then the
same argument applied for every new small bit of code: Not worth adding a
dependency).
So this circles back to the proposal in the thread with "Subject:"
Usage of (Commons) dependencies
That is: _If_ we had a ("very low-level") module that provides the 20 lines
needed by [Configuration] and [Lang], they could both depend on it, with
almost no overhead or additional risks: No undue dependency (Emmanuel's
point), potential fixes would be implemented once (Gary's point).
We could then "manage" this via JIRA:
1. Open a report stating the need for the "very low-level" module and its
scope (i.e. the list of features, such as the "20 lines" referred to here).
2. Open a JIRA sub-task: Replace the "20 lines" once the module is
released.
3. Add a comment around the "20 lines" with a reference to the sub-task.
How does that sound?
Regards,
Gilles
>
> Emmanuel Bourg
>
> [1]
> https://github.com/apache/commons-configuration/commit/02f66818#diff-a8d5e2a5cbeb500e59aca88e0613283c57d530c6df7630555d5a08bc5c3a660fR258
> [2] https://github.com/apache/commons-configuration/commit/8b26e6e6
> [3] https://github.com/apache/commons-configuration/commit/25490ca9
> [4] https://github.com/apache/commons-configuration/commit/63776cc0
> [5] https://github.com/apache/commons-configuration/commit/7b434dbd
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: (commons-configuration) 05/05: Remove the dependency on commons-codec
Posted by Emmanuel Bourg <eb...@apache.org>.
Le 26/10/2024 à 20:32, Elliotte Rusty Harold a écrit :
> That's exactly backwards. Code changes have to be technically
> justified. The responsibility is on the proposer, not the vetoer, to
> demonstrate that the change os worthwhile. Code changes are not
> accepted by default unless there's good and valid reason for them. If
> you can't get even one committer to +1 a PR thgen it doesn't meet the
> threshold for merging.
It goes both ways, you can't veto without a good reason either. "I don't
like it" is definitely not one.
It looks like I have to explain in more details the change, so here it is:
When I implemented the plist configuration in 2005 [1] I needed a Base64
and an hex encoder/decoder. Base64 isn't exactly trivial to implement
and Commons Codec already has one, so I imported the dependency.
The inflation of dependencies was seen as a concern, the plist
configuration initially used Commons Digester which was eventually
replaced by pure SAX code in 2007 [2], Commons Collections was also used
and got removed in 2014 [3]. Some dependencies were made optional to
reduce the amount of dependencies pulled [4], but this made an awkward
user experience where unsuspecting users would get a
ClassNotFoundException when trying to use the plist configurations.
Java 8 was released in 2014 with a long awaited Base64 implementation,
and Commons Configuration moved to Java 8 in 2018. I've updated the code
last week to use java.util.Base64 instead of Commons Codec [5]. At this
point Commons Codec was only retained for encoding/decoding hexadecimal
values. Commons Codec weighs 360KB and must be added manually to the
classpath, encoding/decoding hexadecimal is trivially handled by ~20
lines of code, so dropping the dependency was the obvious choice to me
and in the spirit of the previous dependency removals.
Emmanuel Bourg
[1]
https://github.com/apache/commons-configuration/commit/02f66818#diff-a8d5e2a5cbeb500e59aca88e0613283c57d530c6df7630555d5a08bc5c3a660fR258
[2] https://github.com/apache/commons-configuration/commit/8b26e6e6
[3] https://github.com/apache/commons-configuration/commit/25490ca9
[4] https://github.com/apache/commons-configuration/commit/63776cc0
[5] https://github.com/apache/commons-configuration/commit/7b434dbd
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: (commons-configuration) 05/05: Remove the dependency on commons-codec
Posted by Elliotte Rusty Harold <el...@ibiblio.org>.
On Fri, Oct 25, 2024 at 11:25 PM Emmanuel Bourg <eb...@apache.org> wrote:
>
> Le 25/10/2024 à 12:57, Gary Gregory a écrit :
> > -1, please revert and stop inlining Commons methods all over itself.
>
> Could you elaborate why in this case please? Vetos to code changes have
> to be technically justified. You can't just say that you don't like them.
>
That's exactly backwards. Code changes have to be technically
justified. The responsibility is on the proposer, not the vetoer, to
demonstrate that the change os worthwhile. Code changes are not
accepted by default unless there's good and valid reason for them. If
you can't get even one committer to +1 a PR thgen it doesn't meet the
threshold for merging.
--
Elliotte Rusty Harold
elharo@ibiblio.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: (commons-configuration) 05/05: Remove the dependency on commons-codec
Posted by Gary Gregory <ga...@gmail.com>.
On Fri, Oct 25, 2024 at 7:25 PM Emmanuel Bourg <eb...@apache.org> wrote:
> Le 25/10/2024 à 12:57, Gary Gregory a écrit :
> > -1, please revert and stop inlining Commons methods all over itself.
>
Allow me to restate and elaborate on what I said in the Compress commit
reply and in the thread titled "[All] Usage of (Commons) dependencies": No
copypasta, please. I want to reduce code patterns that are duplicated all
over the place.
In my view, Compress is a higher-level library because it provides an SPI
abstraction over archiving and compression using implementation through
third-party libraries.
Configuration is also a higher-level library that provides an SPI and
different implementations, using third-party libraries like Spring.
In both Compress and Configuration commits, pages and pages of diffs now
duplicate code from other Commons JARs: This makes it impossible to benefit
from bug fixes, performance improvements, and much less CVE fixes. This has
many negative effects: Compress and Configuration are bigger, more complex,
and harder to maintain. Furthermore, it increases the maintenance burden on
Commons overall, if a maintainer fixes an issue in a low-level component
(say Lang), should that maintainer hunt around Commons for copypasta?
Should that maintainer pretend they don't know that copypasta exists?
In the thread "[All] Usage of (Commons) dependencies", Gillies brings up an
interesting tangent: Helping new contributors. One piece of advice I
wouldn't want to give is: "Don't do what we do and copypasta code all over
the place, instead think about how to best design and factor your code
contributions, well expect here and there." I'd rather talk about something
positive, and simpler ;-)
HTH,
Gary
>
> Could you elaborate why in this case please? Vetos to code changes have
> to be technically justified. You can't just say that you don't like them.
>
> In this specific case, Commons Codec was used for the Base64 codec which
> has now a standard equivalent in Java 8, and for the Hex class only used
> in the PList implementations that will be replaced by the Java 17
> HexFormat class. Since Commons Configuration still supports Java 8 I
> suggest using a trimmed down copy of the Commons Codec class to avoid
> the extra dependency.
>
> Ironically, I authored the PList configurations 19 years ago, I hope I
> can still have a say on how they are implemented today :)
>
> Emmanuel Bourg
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
>
Re: (commons-configuration) 05/05: Remove the dependency on commons-codec
Posted by Emmanuel Bourg <eb...@apache.org>.
Le 25/10/2024 à 12:57, Gary Gregory a écrit :
> -1, please revert and stop inlining Commons methods all over itself.
Could you elaborate why in this case please? Vetos to code changes have
to be technically justified. You can't just say that you don't like them.
In this specific case, Commons Codec was used for the Base64 codec which
has now a standard equivalent in Java 8, and for the Hex class only used
in the PList implementations that will be replaced by the Java 17
HexFormat class. Since Commons Configuration still supports Java 8 I
suggest using a trimmed down copy of the Commons Codec class to avoid
the extra dependency.
Ironically, I authored the PList configurations 19 years ago, I hope I
can still have a say on how they are implemented today :)
Emmanuel Bourg
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org