Re: [tomcat-jakartaee-migration] branch master updated (5c96c0b -> 61ba095)
On 07/04/2020 09:40, Emmanuel Bourg wrote: > Le 07/04/2020 à 10:12, Mark Thomas a écrit : > >> The dependencies are only embedded to create a single JAR to make it >> easier for users to use on the command line. If the code was re-used I'd >> expect the "standard" JAR to be used and leave the decision on how to >> handle the dependencies up to the integrator. > > Ok, I assumed the shaded jar was the main artifact. As long as it isn't > published to Maven Central I'm fine with removing the relocation. Working out how this could be formally released is still on the TODO list. I don't view the shaded JAR as the main artefact but I can see it being pushed to Maven Central at some point. If that does happen it would be along side the "standard" JAR which would have a POM with the correct dependencies. Mark - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: [tomcat-jakartaee-migration] branch master updated (5c96c0b -> 61ba095)
Le 07/04/2020 à 10:12, Mark Thomas a écrit : > The dependencies are only embedded to create a single JAR to make it > easier for users to use on the command line. If the code was re-used I'd > expect the "standard" JAR to be used and leave the decision on how to > handle the dependencies up to the integrator. Ok, I assumed the shaded jar was the main artifact. As long as it isn't published to Maven Central I'm fine with removing the relocation. Emmanuel Bourg - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: [tomcat-jakartaee-migration] branch master updated (5c96c0b -> 61ba095)
On 06/04/2020 17:45, Emmanuel Bourg wrote: > Le 06/04/2020 à 18:33, Mark Thomas a écrit : > >> OK. But that is still 7.2k of classes rather than 2.4k of classes for >> zero benefit. >> >> I'll withdraw my -1 because we are approaching the point where the >> differences aren't worth the time spent discussing them but I still >> don't like this change. > > I've removed the duplicated Apache license in the shaded jar, so the > difference is now near zero. > > I agree this change is trivial and doesn't bring significant changes. > Apache Commons was created to share and reuse common code used at the > ASF, I always feel a bit sad when the code produced there isn't reused. > > >> And the relocation of the dependencies? At the moment, I only see a >> downside (harder debugging). What is the benefit? > > Relocating embedded dependencies is a best practice to prevent classpath > conflicts. If the tools ends up being integrated in a bigger software it > could clash with another version of BCEL on the classpath. The dependencies are only embedded to create a single JAR to make it easier for users to use on the command line. If the code was re-used I'd expect the "standard" JAR to be used and leave the decision on how to handle the dependencies up to the integrator. Mark - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: [tomcat-jakartaee-migration] branch master updated (5c96c0b -> 61ba095)
Le 06/04/2020 à 18:33, Mark Thomas a écrit : > OK. But that is still 7.2k of classes rather than 2.4k of classes for > zero benefit. > > I'll withdraw my -1 because we are approaching the point where the > differences aren't worth the time spent discussing them but I still > don't like this change. I've removed the duplicated Apache license in the shaded jar, so the difference is now near zero. I agree this change is trivial and doesn't bring significant changes. Apache Commons was created to share and reuse common code used at the ASF, I always feel a bit sad when the code produced there isn't reused. > And the relocation of the dependencies? At the moment, I only see a > downside (harder debugging). What is the benefit? Relocating embedded dependencies is a best practice to prevent classpath conflicts. If the tools ends up being integrated in a bigger software it could clash with another version of BCEL on the classpath. Emmanuel Bourg - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: [tomcat-jakartaee-migration] branch master updated (5c96c0b -> 61ba095)
On 06/04/2020 17:18, Emmanuel Bourg wrote:
> Le 06/04/2020 à 17:50, Mark Thomas a écrit :
>
>>> from 5c96c0b Ignore the IntelliJ project files
>>> new 29ea189 Replaced NonClosing{In,Out}putStream with the equivalent
>>> classes from Commons IO
>>
>> -1. It adds 220k of bloat for no benefit.
>
> No the dependencies are shaded with the minimizeJar option enabled, only
> the classes used are kept in the final jar.
OK. But that is still 7.2k of classes rather than 2.4k of classes for
zero benefit.
I'll withdraw my -1 because we are approaching the point where the
differences aren't worth the time spent discussing them but I still
don't like this change.
>>> new 528ccfa Made the internal classes package private
>>
>> I am close to -1 on this change.
>>
>> I would rather these were left public at this stage in the development
>> of this tool to make it as easy as possible for folks to tinker with
>> this code, re-using the bits that work for them. Longer term I had the
>> possibility in mind that users might need to register custom Converters
>> so making that package private seems very out of place.
>>
>> I get that we can always relax visibility rules later but this change
>> looks premature to me.
>
> Ok sounds fair, I've reverted it.
And the relocation of the dependencies? At the moment, I only see a
downside (harder debugging). What is the benefit?
Mark
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: [tomcat-jakartaee-migration] branch master updated (5c96c0b -> 61ba095)
Le 06/04/2020 à 17:50, Mark Thomas a écrit :
>> from 5c96c0b Ignore the IntelliJ project files
>> new 29ea189 Replaced NonClosing{In,Out}putStream with the equivalent
>> classes from Commons IO
>
> -1. It adds 220k of bloat for no benefit.
No the dependencies are shaded with the minimizeJar option enabled, only
the classes used are kept in the final jar.
>> new 528ccfa Made the internal classes package private
>
> I am close to -1 on this change.
>
> I would rather these were left public at this stage in the development
> of this tool to make it as easy as possible for folks to tinker with
> this code, re-using the bits that work for them. Longer term I had the
> possibility in mind that users might need to register custom Converters
> so making that package private seems very out of place.
>
> I get that we can always relax visibility rules later but this change
> looks premature to me.
Ok sounds fair, I've reverted it.
Emmanuel Bourg
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: [tomcat-jakartaee-migration] branch master updated (5c96c0b -> 61ba095)
On 06/04/2020 14:34, ebo...@apache.org wrote:
> This is an automated email from the ASF dual-hosted git repository.
>
> ebourg pushed a change to branch master
> in repository
> https://gitbox.apache.org/repos/asf/tomcat-jakartaee-migration.git.
>
>
> from 5c96c0b Ignore the IntelliJ project files
> new 29ea189 Replaced NonClosing{In,Out}putStream with the equivalent
> classes from Commons IO
-1. It adds 220k of bloat for no benefit.
> new 528ccfa Made the internal classes package private
I am close to -1 on this change.
I would rather these were left public at this stage in the development
of this tool to make it as easy as possible for folks to tinker with
this code, re-using the bits that work for them. Longer term I had the
possibility in mind that users might need to register custom Converters
so making that package private seems very out of place.
I get that we can always relax visibility rules later but this change
looks premature to me.
> new 61ba095 Relocated the dependencies in the shaded jar
Again, I am close to -1 on this change.
Why? I don't see the need for this. It just makes debugging potentially
harder.
Mark
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
[tomcat-jakartaee-migration] branch master updated (5c96c0b -> 61ba095)
This is an automated email from the ASF dual-hosted git repository.
ebourg pushed a change to branch master
in repository
https://gitbox.apache.org/repos/asf/tomcat-jakartaee-migration.git.
from 5c96c0b Ignore the IntelliJ project files
new 29ea189 Replaced NonClosing{In,Out}putStream with the equivalent
classes from Commons IO
new 528ccfa Made the internal classes package private
new 61ba095 Relocated the dependencies in the shaded jar
The 3 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails. The revisions
listed as "add" were already present in the repository and have only
been added to this reference.
Summary of changes:
pom.xml| 15
.../apache/tomcat/jakartaee/ClassConverter.java| 2 +-
.../org/apache/tomcat/jakartaee/Converter.java | 2 +-
.../java/org/apache/tomcat/jakartaee/Info.java | 2 +-
.../org/apache/tomcat/jakartaee/Migration.java | 7 +-
.../org/apache/tomcat/jakartaee/NoOpConverter.java | 2 +-
.../tomcat/jakartaee/NonClosingInputStream.java| 84 --
.../tomcat/jakartaee/NonClosingOutputStream.java | 60
.../org/apache/tomcat/jakartaee/StringManager.java | 2 +-
.../org/apache/tomcat/jakartaee/TextConverter.java | 2 +-
.../java/org/apache/tomcat/jakartaee/Util.java | 2 +-
11 files changed, 27 insertions(+), 153 deletions(-)
delete mode 100644
src/main/java/org/apache/tomcat/jakartaee/NonClosingInputStream.java
delete mode 100644
src/main/java/org/apache/tomcat/jakartaee/NonClosingOutputStream.java
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
