Move to nowarn instead of silencer #29932

andreaTP · 2021-01-07T08:32:40Z

Follow up from:
#29929 (comment) (#29929)

Apart from the temporary workaround for Scala 2.12 I just validate that we can use the same approach for Scala 3.
Thanks, @ghik and @raboof

andreaTP · 2021-01-07T08:35:05Z

Side note:
I wanted to reformat just my changes to /project/*.scala with scalafmtSbt and ended up reformatting everything there, I hope is not a problem, in case I can revert.
Not sure where, but we probably want to add sbt scalafmtSbtCheck to the CI.

akka-ci · 2021-01-07T08:36:22Z

Thank you for your pull request! After a quick sanity check one of the team will reply with 'OK ΤO ΤESΤ' to kick off our automated validation on Jenkins. This compiles the project, runs the tests, and checks for things like binary compatibility and source code formatting. When two team members have also manually reviewed and (perhaps after asking for some amendments) accepted your contribution, it should be good to be merged.

For more details about our contributing process, check out CONTRIBUTING.md - and feel free to ask!

raboof · 2021-01-07T08:40:50Z

OK TO TEST

raboof · 2021-01-07T08:49:13Z

project/AkkaDisciplinePlugin.scala

+        "-P:silencer:globalFilters=The outer reference in this type test cannot be checked at run time",
+        // Because we show some things that are deprecated in
+        // 2.13 but don't have a replacement that was in 2.12:
+        "-P:silencer:globalFilters=deprecated \\(since 2.13.0\\)"))


It seems Scala 2.13 doesn't like these settings - are there equivalent settings to configure 'nowarn'?

I will fix it

akka-ci · 2021-01-07T09:57:00Z

Test PASSed.

andreaTP · 2021-01-08T10:30:32Z

project/AkkaDisciplinePlugin.scala

+          ("com.github.ghik" %% "silencer-lib" % silencerVersion % Provided).cross(CrossVersion.patch))
+        Seq(libraryDependencies ++= (if (autoScalaLibrary.value) libs else Nil))
+      case _ =>
+        Seq(scalacOptions ++= Seq("-nowarn", "-Wconf:any:e"))


I have tried a bunch of patterns, but I have troubles and I might be falling into bugs.
E.g.
With this configuration: sbt -Dakka.build.scalaVersion=2.13 akka-actor/test:compile result in 12 errors in akka-actors all in:
@nowarn annotation does not suppress any warnings

The full scalacConfiguration is:

akka > show akka-actor/test:scalacOptions [info] * -nowarn [info] * -Wconf:any:e [info] * -Xfatal-warnings [info] * -Ywarn-extra-implicit [info] * -Ywarn-unused:_ [info] * -Xlint [info] * -deprecation [info] * -encoding [info] * UTF-8 [info] * -feature [info] * -unchecked [info] * -language:higherKinds [info] * -target:jvm-1.8

It would be nice to get help from someone like @lrytz 🙏

👋 the compiler treats -Wconf before -nowarn. Using both -nowarn -Wconf:any:e first promotes all warnings to be errors, so the messages are issued using reporter.error, and -nowarn has no effect anymore as it's checked in reporter.warning.

@nowarn annotation does not suppress any warnings is a warning enabled by -Xlint, it means that a @nowarn annotation is redundant, i.e., doesn't silence anything. It is reported as an error here because of -Wconf:any:e.

Does that help..?

Indeed, thanks a lot @lrytz I found my way out!
1437da6
https://github.com/akka/akka/pull/29932/checks?check_run_id=1669212181

akka-ci · 2021-01-08T11:31:56Z

Test PASSed.

akka-ci · 2021-01-08T15:50:41Z

Test PASSed.

patriknw · 2021-01-09T14:20:19Z

akka-actor/src/main/scala-2.12/scala/annotation/nowarn.scala

+
+// TODO: remove this when upgrading to Scala 2.12.13
+// https://github.com/scala/scala/pull/9248
+package scala.annotation


will this work with OSGi? I think it doesn't allow source in other packages than the root package of the module.
Eventually we will remove the OSGi support #28304 but we are not there yet.

How can we test?
I was working under the assumption that, if the tests are passing, I'm not breaking anything.

That said, I'm not sure how can OSGi detect this, at best of my understanding:

Akka depends on Scala and scala library should be on the classpath

I'm not aware of a way to distinguish from which jar a specific class is coming from given a classpath

@nowarn is an annotation and should not spill to runtime

Am I making any wrong assumptions @patriknw ? I'm certainly not an OSGi expert ...

andreaTP · 2021-01-13T13:11:26Z

Bumped Scala 2.12 to 2.12.13 that has nowarn.
I kept the pattern matching on the version since I'm going to use that structure sooner, is it ok?
This also removes the dependency on silencer.

akka-ci · 2021-01-13T13:12:16Z

Test FAILed.

akka-ci · 2021-01-13T15:53:35Z

Test FAILed.

akka-ci · 2021-01-13T17:56:18Z

Test FAILed.

andreaTP · 2021-01-13T17:59:35Z

Flaky?
https://jenkins.akka.io:8498/job/pr-validator-per-commit-jenkins/21312/

akka-ci · 2021-01-13T19:21:30Z

Test PASSed.

andreaTP · 2021-01-14T08:24:43Z

project/AkkaDisciplinePlugin.scala

@@ -63,17 +63,39 @@ object AkkaDisciplinePlugin extends AutoPlugin {
    "akka-stream-tests-tck",
    "akka-testkit")

-  lazy val silencerSettings = {


This is the relevant diff for reviewers.

andreaTP · 2021-01-14T08:25:36Z

Ready for a final review.

raboof

neat!

raboof · 2021-01-14T08:51:29Z

project/Dependencies.scala

@@ -37,19 +37,21 @@ object Dependencies {
  val scalaTestVersion = "3.1.4"
  val scalaCheckVersion = "1.15.1"

+    def getScalaVersion() = {


Suggested change

def getScalaVersion() = {

def getScalaVersion() = {

the formatting is done by scalafmtSbt, should I change it?

are you sure? sbt scalafmtSbt seems to change it for me as well

I'll re-run it so 👍

scalafmt was breaking, fixed, and re-formatted

raboof · 2021-01-14T08:52:31Z

project/Dependencies.scala

@@ -37,19 +37,21 @@ object Dependencies {
  val scalaTestVersion = "3.1.4"
  val scalaCheckVersion = "1.15.1"

+    def getScalaVersion() = {
+    // don't allow full override to keep compatible with the version of silencer


I guess this comment doesn't apply anymore as we don't use the silencer anymore?

Good catch! This is an interesting subject, in the Scala 3 PR I'm heavily relying on this method, even if it looks like is not needed anymore, can I just remove the comment for now?

raboof · 2021-01-14T08:53:35Z

project/Dependencies.scala

+    // don't allow full override to keep compatible with the version of silencer
+    // don't mandate patch not specified to allow builds to migrate
+    System.getProperty("akka.build.scalaVersion", "default") match {
+      case twoThirteen if twoThirteen.startsWith("2.13") => scala213Version


(a bit weird but it was already like this so 👍 :) )

I can include any encoding you prefer, I found it weird too :)

I suspect we still have jenkins jobs that pass 2.13.x as akka.build.scalaVersion, and we wanted to fix those before changing the startsWith match to an exact match. Might be best to cover in another PR?

akka-ci · 2021-01-14T11:15:02Z

Test PASSed.

patriknw · 2021-01-25T12:07:59Z

Superseded by #29956, I assume

akka-ci added the validating PR is currently being validated by Jenkins label Jan 7, 2021

raboof reviewed Jan 7, 2021

View reviewed changes

akka-ci added tested PR that was successfully built and tested by Jenkins and removed validating PR is currently being validated by Jenkins labels Jan 7, 2021

akka-ci added validating PR is currently being validated by Jenkins and removed tested PR that was successfully built and tested by Jenkins labels Jan 8, 2021

andreaTP commented Jan 8, 2021

View reviewed changes

akka-ci added tested PR that was successfully built and tested by Jenkins and removed validating PR is currently being validated by Jenkins labels Jan 8, 2021

akka-ci added validating PR is currently being validated by Jenkins tested PR that was successfully built and tested by Jenkins and removed tested PR that was successfully built and tested by Jenkins validating PR is currently being validated by Jenkins labels Jan 8, 2021

patriknw reviewed Jan 9, 2021

View reviewed changes

andreaTP mentioned this pull request Jan 11, 2021

Scala3 support patch 1 #29930

Closed

akka-ci added validating PR is currently being validated by Jenkins needs-attention Indicates a PR validation failure (set by CI infrastructure) and removed tested PR that was successfully built and tested by Jenkins validating PR is currently being validated by Jenkins labels Jan 13, 2021

andreaTP added 3 commits January 13, 2021 13:41

Move to nowarn instead of silencer

36ee971

before starting heavy changes

268cbf5

bug?

5b42a85

Cross compatible annotations

bbc85a6

fix docs generation

f850883

minor

53ecab9

akka-ci added validating PR is currently being validated by Jenkins tested PR that was successfully built and tested by Jenkins and removed needs-attention Indicates a PR validation failure (set by CI infrastructure) validating PR is currently being validated by Jenkins labels Jan 13, 2021

andreaTP commented Jan 14, 2021

View reviewed changes

raboof approved these changes Jan 14, 2021

View reviewed changes

scalafmtSbt

d08a9bc

akka-ci added validating PR is currently being validated by Jenkins tested PR that was successfully built and tested by Jenkins and removed tested PR that was successfully built and tested by Jenkins validating PR is currently being validated by Jenkins labels Jan 14, 2021

patriknw closed this Jan 25, 2021

Move to nowarn instead of silencer #29932

Move to nowarn instead of silencer #29932

Conversation

andreaTP commented Jan 7, 2021

andreaTP commented Jan 7, 2021

akka-ci commented Jan 7, 2021

raboof commented Jan 7, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akka-ci commented Jan 7, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akka-ci commented Jan 8, 2021

akka-ci commented Jan 8, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreaTP commented Jan 13, 2021

akka-ci commented Jan 13, 2021

akka-ci commented Jan 13, 2021

akka-ci commented Jan 13, 2021

andreaTP commented Jan 13, 2021

akka-ci commented Jan 13, 2021

Choose a reason for hiding this comment

andreaTP commented Jan 14, 2021

raboof left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akka-ci commented Jan 14, 2021

patriknw commented Jan 25, 2021