search

apache / pekko-http

The Streaming-first HTTP server/module of Apache Pekko
https://pekko.apache.org/
Apache License 2.0
149 stars 36 forks source link

consider using parboiled2 as a jar dependency in v1.0.0 #39

Closed pjfanning closed 1 year ago

pjfanning commented 1 year ago

Relates to https://github.com/apache/incubator-pekko-http/issues/35

pjfanning commented 1 year ago

@jrudolph @mdedetrich this is just for discussion but I'd be interested in your views on this

mdedetrich commented 1 year ago

one particularly problematic file is https://github.com/apache/incubator-pekko-http/blob/2e8b2fade52cf0a85d0150a407226851cf00c683/akka-parsing/src/main/java/akka/parboiled2/util/Base64.java

  • has multiple authors at various stages and BSD and Apache licensing
  • even maybe possible to use Java's Base64 class at this stage

I will comment on the other stuff later, but I wouldn't advocate for removing/changing this for Java's Base64 class. There is a reason why this Base64 implementation is there, Base64 is used frequently in the context of http and as you can read from the comments its a far more efficient implementation then Java's Base64. Generally speaking its also not unusual for the Akka/Pekko ecosystem to reimplement JVM API's if they are not very efficient (which isn't uncommon) and one of Akka/Pekko's main selling points is performance.

As an alternative we can make this base64 into its own sbt module/artifact which would make it fairly easy to add in custom licenses in a similar to what was done with pekko-core's protobuf modules? This can also provide other benefits, i.e. other projects can include specifically just the base64 artifact rather than entire pekko-http if thats all they need.

jrudolph commented 1 year ago

This is a duplicate of #1. Basically, I agree with this move. In #14, @mdedetrich suggested to move that change to the 1.1.x release but if it is easy enough to do I would also concur with doing this earlier if it simplifies the license situation.

jrudolph commented 1 year ago

Btw. IIRC the original reasoning for vendoring was to avoid a transitive dependency on shapeless (which has now been removed from parboiled2) to avoid having to delay releases until shapeless is out.

mdedetrich commented 1 year ago

@jrudolph Do you have any comments on the Base64 situation?

mdedetrich commented 1 year ago

In https://github.com/apache/incubator-pekko-http/pull/14, @mdedetrich suggested to move that change to the 1.1.x release but if it is easy enough to do I would also concur with doing this earlier if it simplifies the license situation.

I am fine with doing the change in 1.0.x release if it simplifies the license situation dramatically. pekko-http due to being a http web server/client doesn't have as strict requirements as something like pekko-core

pjfanning commented 1 year ago

I'll close this in favour of https://github.com/apache/incubator-pekko-http/issues/1 and link this discussion there

mdedetrich commented 1 year ago

Also created https://github.com/apache/incubator-pekko-http/issues/40 to discuss Base64 situation specifically