ensure scala-3 branch ends up with all the changes on main branch

pjfanning commented 1 year ago

scala-3 branch is basically a fork of main branch but with support for Scala3.
it has been updated with most of the recent commits to main (cherry-picked or an equivalent commit if the cherry pick did not work)
one major commit that needs to be duplicated in scala-3 branch is https://github.com/apache/incubator-pekko-http/commit/56158e46960d601437fda04a726ed81162bd3b2b -- this one did not cherry pick well, huge conflicts
- I don't know of any other missing Pekko era commits that are not brought across
there might be other commits that were missed from Akka times (ie from before Pekko work started)

pjfanning commented 1 year ago

@mdedetrich since you made the https://github.com/apache/incubator-pekko-http/commit/56158e46960d601437fda04a726ed81162bd3b2b changes, would you be a position to do an equivalent change for the 'scala-3' branch so that that branch does not become stale?

mdedetrich commented 1 year ago

@mdedetrich since you made the https://github.com/apache/incubator-pekko-http/commit/56158e46960d601437fda04a726ed81162bd3b2b changes, would you be a position to do an equivalent change for the 'scala-3' branch so that that branch does not become stale?

Will do, I actually wanted to do one additional PR to change the package to org.apache.pekko, since we are starting to have PRs against pekko http I want to reduce merge conflicts

jrudolph commented 1 year ago

Is there a good reason to use cherry picking for that branch? I think we should merge the main branch back to the scala-3 branch from time to time. Otherwise, the final merge will be a big problem.

mdedetrich commented 1 year ago

I was imaging a merge as well. I haven't looked into this because of https://github.com/apache/incubator-pekko/issues/107, more specifically https://github.com/apache/incubator-pekko-http/issues/21

raboof commented 1 year ago

I started on bringing a branch up to date by merging main (in chunks), WIP is at https://github.com/apache/incubator-pekko-http/compare/main...raboof:incubator-pekko-http:scala-3-merged .

I got stuck at merging 26846a02a116993138365405354efc45cb49b563 - perhaps @luksow can spot what goes wrong there (when you build with sbt ++3.2.1 compile).

mdedetrich commented 1 year ago

@raboof Not sure if entirely relevant but Is there a reason why you are using Scala 3.2.1? I was under the impression that for now we are compiling against Scala 3.1.1 since thats what Pekko is built against (see https://github.com/apache/incubator-pekko/blob/main/project/Dependencies.scala#L57). Note that there is a PR that will update Pekko to use Scala 3.2.2 (see https://github.com/apache/incubator-pekko/pull/273) but it hasn't been merged yet.

I also checked git blame for parboiled2 (see https://github.com/sirthias/parboiled2/blame/master/build.sbt#L5) and I think the version that pekko-http is using right now is also built against 3.2.x when correlated with the release date (see https://mvnrepository.com/artifact/org.parboiled/parboiled_3/2.4.1).

Will also update assignees

jrudolph commented 1 year ago

Not sure if entirely relevant but Is there a reason why you are using Scala 3.2.1?

parboiled2 is on 3.2.1 so we have to be, too.

jrudolph commented 1 year ago

Note that there is a PR that will update Pekko to use Scala 3.2.2 (see apache/incubator-pekko#273) but it hasn't been merged yet.

This does not really matter right now as Scala 3 is backward compatible but not forward. You can use pekko core with a lower Scala 3 version than pekko-http.

mdedetrich commented 1 year ago

This does not really matter right now as Scala 3 is backward compatible but not forward. You can use pekko core with a lower Scala 3 version than pekko-http.

Okay then its unrelated. There is a version of Parboiled that was released for Scala 3.1.1 (version 2.4.0 specifically) but in any case Pekko will very likely get updated to use Scala 3.2.1 as mentioned before.

jrudolph commented 1 year ago

Here's a diff to fix the immediate problem:

diff --git a/http-core/src/main/scala/org/apache/pekko/http/impl/model/parser/CommonRules.scala b/http-core/src/main/scala/org/apache/pekko/http/impl/model/parser/CommonRules.scala
index 49b4e05311..12223e5a12 100644
--- a/http-core/src/main/scala/org/apache/pekko/http/impl/model/parser/CommonRules.scala
+++ b/http-core/src/main/scala/org/apache/pekko/http/impl/model/parser/CommonRules.scala
@@ -22,7 +22,9 @@ import pekko.http.scaladsl.model.headers._
 import org.parboiled2._
 import org.parboiled2.support.hlist._

-private[parser] trait CommonRules { this: HeaderParser with Parser with StringBuilding =>
+private[parser] trait CommonRules extends StringBuilding { this: Parser =>
+  protected def maxCommentParsingDepth: Int
+
   import CharacterClasses._

   // ******************************************************************************************

Though, there's another minor problem in the tests.

jrudolph commented 1 year ago

I think we can just remove the test failing to compile in HttpHeaderSpec regarding If-Match dispatching, it's not relevant any more anyway.

pjfanning commented 1 year ago

https://github.com/apache/incubator-pekko-http/pull/130 is merged to main branch

apache / pekko-http

ensure scala-3 branch ends up with all the changes on main branch #17