Scala2.12 support

[pjfanning]

S3 code needs a lot of changes to compile in Scala. 2.12

[nvollmar]

lgtm

[pjfanning]

@mdedetrich could you have a look?

In Alpakka, they removed scala 2.12 support over a year ago. We're supposed to support Scala 2.12. The most affected code is in the S3 module that you have been the main recent contributor to.

• we could choose to just use plain Seqs instead of immutable.Seqs which would make the code more easily compiled in Scala 2.12 and Scala 2.13
• I'm using toIndexedSeq to convert plain Seqs to immutable.Seqs (the function returns an immutable.IndexedSeq) - a function that is readily available in the SDK)
• or I could introduce code to convert plain Seqs to immutable.Seqs (a no-op in Scala 2.13 and for Scala 2.12, I could provide an optimised

[mdedetrich]

I'll have a look at this tonight

[mdedetrich]

At first glance if this problem is what I think it is (i.e. Scala 2.13 having a stdlib collection rewrite which changed some interfaces) then the best course of action would be to use https://github.com/scala/scala-collection-compat. The whole idea of this library is it provides common interfaces to various Scala versions to help deal with this problem.

Its also possible to just inline the abstractions if we don't want to add an additional dependency, although I would argue that scala-collection-compat is ultra stable at this point.

@pjfanning Do you want to me work on this?

[pjfanning]

I'm not convinced by the need for scala-collection-compat. I've even reduced the uses of toIndexedSeq to the point that most have pre-existed the commit that removed Scala 2.12 support. 49ec3aa

[mdedetrich]

I am not sure if using immutable.Seq is desirable here because the point of using normal Seq is that it also accepts mutable collections which can be relevant from the Java side of things.

Also note that other akka/pekko libraries have abstracted over these interfaces (such as IndexedSeq) in a similar way to scala-collection-compat but in a much more simple way (and also to avoid a dependency) and to me I think this is preferable compared to changing the core collection types.

I have to look at this more concretely though.

[mdedetrich]

Also note that other akka/pekko libraries have abstracted over these interfaces (such as IndexedSeq) in a similar way to scala-collection-compat but in a much more simple way (and also to avoid a dependency)

So I just found where this is the case i.e. https://github.com/apache/incubator-pekko/blob/32f7f1118ecbccbdc80625257afb44d8b06e495e/actor/src/main/scala-2.12/org/apache/pekko/util/ccompat/CompatImpl.scala

[pjfanning]

I can double check but I don't think this PR affects any public APIs. The changes are in private or package private code. There is a broken unit test though. I'll debug it later.

If you want to try Scala-collection-compat, give it a go. We can see if the code that gets used is small and decide afterwards whether to inline that code or not.

[mdedetrich]

Okay so I had a proper look at the changes regarding immutable.Seq and it is indeed safe. This was due to another change where in Scala 2.13 they changed Seq to mean immutable.Seq.

I noticed there are some other refactoring's in the PR, would it be possible to split them out into the PR (unless they are necessary for 2.12 which I don't think is the case?). This is firstly to keep the scope of PR's small but also in the interest of making 1.0.x as similar to Alpakka as possible incase there are some backports (completely fine with putting this change in 1.1.x).

Also the S3 tests are now failing (they werent before) and I suspect this may be due to these changes breaking a mocking test.

[pjfanning]

@mdedetrich I built the PR by reverting back to the changes before Scala 2.12 was dropped and selectively adding changes. Nonetheless, I revisited the changes and added back more of the changes so the PR is smaller now. These changes also fixed the unit test that was failing.

[mdedetrich]

I didn't realize that you were bringing in changes from some time back. Irregardless PR is looking much better.

Only concerning thing I see is the .toIndexedSeq calls and some other manual translations of collections which I ideally would like to avoid.

On mobile right now so will check it out tomorrow.

[mdedetrich]

@pjfanning Thanks for the PR. These changes are very good, we actually made the code slightly faster due to removing unnecessary conversions.

I added 2 commits onto the PR, one of them fixes on last case of a .toIndexedSeq which wasn't necessary (it was a workaround for forgetting to use immutable.Seq in another area).

The second commit I added removes all warnings due to the Scala cross compiling that we just fixed. Feel free to merge the PR if there aren't any additional changes needed.

[mdedetrich]

This is needed because we use the @nowarn annotation to suppress a warning only for Scala 2.13 and without this scalac option it would complain that no warnings were suppressed for Scala 2.12.