Skip to content

KAFKA-7197 expand gradle build: include Scala 2.13 #5454

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jun 22, 2019

Conversation

dejan2609
Copy link
Contributor

@dejan2609 dejan2609 commented Aug 2, 2018

Final PR description (June 2019):

  • include Scala 2.13 into gradle build
  • handle future milestone and RC versions of Scala in a better way
  • if no Scala version is specified, default to scala 2.12 (bump from 2.11)
  • include certain Xlint options (removed by Scala 2.13) for Scala 2.11/2.12 build only
  • upgrade versions for dependencies:
    • scalaLogging: 3.9.0 -->> 3.9.2
    • scalatest: 3.0.7 -->> 3.0.8
    • scoverage: 1.3.1 -->> 1.4.0

Initial PR description (August 2018):

details:

  • build expanded in order to use Scala 2.13 milestone version (version 2.13.0-M3 is used)
  • 'scoverage' version upgraded: 1.3.1 -->> 1.4.0-M3

Note: ./gradlew -PscalaVersion=2.13 jar -q returns few errors (well, as expected):

8 errors found

FAILURE: Build failed with an exception.

* What went wrong:
Execution failed for task ':core:compileScala'.
> Compilation failed

Upgrade to Scala 2.13.0-M4 depends on:

@ijuma
Copy link
Member

ijuma commented Aug 8, 2018

Thanks for the PR. M5 is due soon, I think we should go straight to that in preparation of RC1. We should probably not build it by default until the final release.

@ijuma
Copy link
Member

ijuma commented Sep 8, 2018

@dejan2609 2.13.0-M5 has been released. Maybe update this PR to use that?

@dejan2609
Copy link
Contributor Author

@ijuma Sure, sorry for my late answer (I was traveling). I will try to update PR asap.

@dejan2609
Copy link
Contributor Author

Update / related PR:
scoverage/scalac-scoverage-plugin#234 Add Scala 2.13.0-M5 support

@dejan2609
Copy link
Contributor Author

Update 2: related PR mentioned above is solved, I will try to accommodate this PR accordingly.

@dejan2609
Copy link
Contributor Author

Long overdue, I will try to squeeze this in a next few days.
FYI @ijuma

@dejan2609
Copy link
Contributor Author

Ok, I have some solution, but I need to hack Xlint options in addition, suppress unused import statements temporarily and so on (after that I will push some solution here):

dejans@BEG-IT-09 MINGW64 /c/Work/IDEA-2019.1/workspace/kafka (trunk)
$ ./gradlew -x test -PscalaVersion=2.13 jar -q
Building project 'core' with Scala version 2.13.0-M5
Building project 'streams-scala' with Scala version 2.13.0-M5
'by-name-right-associative' is not a valid choice for '-Xlint'
'unsound-match' is not a valid choice for '-Xlint'
<=------------> 14% EXECUTING [14s]
> :core:compileScala

dejans@BEG-IT-09 MINGW64 /c/Work/IDEA-2019.1/workspace/kafka (trunk)
$ ./gradlew -x test -PscalaVersion=2.12 jar -q
Building project 'core' with Scala version 2.12.8
Building project 'streams-scala' with Scala version 2.12.8
<=------------> 14% EXECUTING [9s]
> :core:compileScala

dejans@BEG-IT-09 MINGW64 /c/Work/IDEA-2019.1/workspace/kafka (trunk)
$ date
Tue Feb 26 08:25:43 CEST 2019

dejans@BEG-IT-09 MINGW64 /c/Work/IDEA-2019.1/workspace/kafka (trunk)

@dejan2609
Copy link
Contributor Author

Ok, I pushed commit (after quite a while).
Build passes locally for Scala 2.11 and 2.12 and (unsurprisingly) fails for Scala 2.13 with bunch of errors.

@ijuma now I see that I forgot to comply with your comment above (2.13 should not be listed among defaultScalaVersions yet... I will fix that).

@dejan2609
Copy link
Contributor Author

dejan2609 commented Feb 26, 2019

Just to sum it up:

Let me know what you think @ijuma @ewencp

@dejan2609 dejan2609 changed the title [WIP] KAFKA-7197 Scala '2.13-milestone' experimental build (gradle build improvement) KAFKA-7197 Scala '2.13-milestone' experimental build (gradle build improvement) Mar 1, 2019
@dejan2609
Copy link
Contributor Author

retest this please

1 similar comment
@dejan2609
Copy link
Contributor Author

retest this please

@dejan2609
Copy link
Contributor Author

@ijuma please review (note: there are some flaky test).

@dejan2609
Copy link
Contributor Author

@ijuma please review.

@dejan2609
Copy link
Contributor Author

Thanx @ewencp for your review(s) above, I will try to implement/modify based on your suggestions.

@dejan2609
Copy link
Contributor Author

dejan2609 commented Mar 20, 2019

New commit pushed. @ewencp and @ijuma: feel free to check and review.

I added related comment(s) that could be seen as an overzealous/overkill (will make them short if required or move them into commit message).

@dejan2609
Copy link
Contributor Author

@dejan2609
Copy link
Contributor Author

Related: scoverage/scalac-scoverage-plugin#250 Add Scala 2.13.0-RC1 support

@dejan2609
Copy link
Contributor Author

Update: scalac-scoverage-plugin and scalac-scoverage-runtime versions for 2.13.0-RC1 are released.

scala-logging release is pending:

Copy link
Member

@ijuma ijuma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR. I left some minor comments. Apart from that:

  1. Please rebase as we just upgraded the Gradle version in trunk.
  2. I think we can go straight to 2.13.0-RC1 since it seems like most libraries are already there.

Hopefully we can enable 2.13 by default for Kafka 2.4.

@ennru
Copy link

ennru commented Jun 12, 2019

Scalatest 3.0.8 and scala-logging 3.9.2 are available for Scala 2.13.0 now. Getting there...

@dejan2609
Copy link
Contributor Author

Ok, all dependencies are here, but I just digged some warnings that should be suppressed (by-name-right-associative and unsound-match made their way into Scala 2.13 build, I will have to investigate that).

@dejan2609
Copy link
Contributor Author

dejan2609 commented Jun 13, 2019

Update: switch from if ((versions.baseScala = '2.11') || (versions.baseScala = '2.12')) to if (versions.baseScala in ['2.11','2.12']) did the trick (and it looks better, I guess).

Build works for Scala 2.11 / 2.12 and (unsurprisingly) returns some errors for Scala 2.13: ./gradlew clean -PscalaVersion=2.13 jar -q

151 errors found

FAILURE: Build failed with an exception.

* What went wrong:
Execution failed for task ':core:compileScala'.
> Compilation failed

@ennru
Copy link

ennru commented Jun 14, 2019

Great!
So what would be the plan here:

  1. Will the build updates get merged and the code changes be worked on another PR, or
  2. Should someone else branch this PR and suggest how to adapt to the Scala 2.13 (collection) APIs?

@dejan2609
Copy link
Contributor Author

Thanx @ennru, I was just about to ask the same question.

@ijuma, @ewencp: what is your take on this ?

@dejan2609
Copy link
Contributor Author

retest this please

@dejan2609
Copy link
Contributor Author

Pinging @ijuma and @ewencp again.

@ijuma
Copy link
Member

ijuma commented Jun 19, 2019

@ennru We have to support Scala 2.11, 2.12 and 2.13. I don't expect an immediate code change to adapt to 2.13 collection APIs. Maybe further down the line when we drop 2.11 and consider switching to 2.13 as the recommended version (we'd still have to be compatible with 2.12 at that point).

@ijuma
Copy link
Member

ijuma commented Jun 19, 2019

retest this please

@dejan2609
Copy link
Contributor Author

@ijuma solved, rebased, squashed and force-pushed.

@ijuma
Copy link
Member

ijuma commented Jun 22, 2019

@dejan2609 Can you please rebase (unfortunately your branch includes a change that broke one test, it has since been fixed) and update the PR description to match the current reality? I should be able to merge then.

Copy link
Member

@ijuma ijuma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, noticed a couple of additional changes. We should be good to go after that.

Copy link
Member

@ijuma ijuma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made a couple of changes and it LGTM. It just needs a rebase and for the PR description to be updated.

@dejan2609 dejan2609 changed the title KAFKA-7197 Scala '2.13-milestone' experimental build (gradle build improvement) KAFKA-7197 expand gradle build: include Scala 2.13 Jun 22, 2019
…d default Scala versions

details/notes:
  * gradle build now includes Scala 2.13; Scala 2.11/2.12 builds are intact
  * if no Scala version is specified, default to scala 2.12 (was 2.11)
  * 'baseScala' version resolving is expanded in order to handle milestone and RC versions of Scala
  * Scala 2.13 removes some scalac options and hence gradle build will include certain Xlint options ('by-name-right-associative' and 'unsound-match') for Scala 2.11/2.12 only
  * 'README.md' is changed accordingly
  * required version upgrades for dependencies:
        ** scalaLogging: 3.9.0 -->> 3.9.2
        ** scalatest:    3.0.7 -->> 3.0.8
        ** scoverage:    1.3.1 -->> 1.4.0
@dejan2609
Copy link
Contributor Author

@ijuma Done as requested, all changes are bundled into one commit.
I took the liberty to alter PR description in a way that list both initial and final state (given a fact that PR exists for a while).

@ijuma
Copy link
Member

ijuma commented Jun 22, 2019

Scala 2.12 is green, Scala 2.11 had 1 unrelated flaky connect test. Merging to master.

@ijuma ijuma merged commit 5339d2d into apache:trunk Jun 22, 2019
@ijuma
Copy link
Member

ijuma commented Jun 23, 2019

While updating Jenkins to build with Scala 2.13, I ran into a number of compiler errors. I'll submit a PR with fixes.

@dejan2609 dejan2609 deleted the develop branch June 23, 2019 11:37
@ijuma
Copy link
Member

ijuma commented Jun 24, 2019

See #6989

ijuma added a commit that referenced this pull request Jul 2, 2019
Scala 2.13 support was added to build via #5454. This PR adjusts the code so that
it compiles with 2.11, 2.12 and 2.13.

Changes:
* Add `scala-collection-compat` dependency.
* Import `scala.collection.Seq` in a number of places for consistent behavior between
Scala 2.11, 2.12 and 2.13.
* Remove wildcard imports that were causing the Java classes to have priority over the
Scala ones, related Scala issue: scala/scala#6589.
* Replace parallel collection usage with `Future`. The former is no longer included by
default in the standard library.
* Replace val _: Unit workaround with one that is more concise and works with Scala 2.13
* Replace `filterKeys` with `filter` when we expect a `Map`. `filterKeys` returns a view
that doesn't implement the `Map` trait in Scala 2.13.
* Replace `mapValues` with `map` or add a `toMap` as an additional transformation
when we expect a `Map`. `mapValues` returns a view that doesn't implement the
`Map` trait in Scala 2.13.
* Replace `breakOut` with `iterator` and `to`, `breakOut` was removed in Scala
2.13.
* Replace to() with toMap, toIndexedSeq and toSet
* Replace `mutable.Buffer.--` with `filterNot`.
* ControlException is an abstract class in Scala 2.13.
* Variable arguments can only receive arrays or immutable.Seq in Scala 2.13.
* Use `Factory` instead of `CanBuildFrom` in DecodeJson. `CanBuildFrom` behaves
a bit differently in Scala 2.13 and it's been deprecated. `Factory` has the behavior
we need and it's available via the compat library.
* Fix failing tests due to behavior change in Scala 2.13,
"Map.values.map is not strict in Scala 2.13" (scala/bug#11589).
* Use Java collections instead of Scala ones in StreamResetter (a Java class).
* Adjust CheckpointFile.write to take an `Iterable` instead of `Seq` to avoid
unnecessary collection copies.
* Fix DelayedElectLeader to use a Map instead of Set and avoid `to` call that
doesn't work in Scala 2.13.
* Use unordered map for mapping in SimpleAclAuthorizer, mapping of ordered
maps require an `Ordering` in Scala 2.13 for safety reasons.
* Adapt `ConsumerGroupCommand` to compile with Scala 2.13.
* CoreUtils.min takes an `Iterable` instead of `TraversableOnce`, the latter does
not exist in Scala 2.13.
* Replace `Unit` with `()` in a couple places. Scala 2.13 is stricter when it expects
a value instead of a type.
* Fix bug in CustomQuotaCallbackTest where we did not necessarily set `partitionRatio`
correctly, `forall` can terminate early.
* Add a couple of spotbugs exclusions that are needed by code generated by Scala 2.13
* Remove unused variables, simplify some code and remove procedure syntax in a few
places.
* Remove unused `CoreUtils.JSONEscapeString`.

Reviewers: Manikumar Reddy <[email protected]>, José Armando García Sancio <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants