Skip to content

Commit

Permalink
Add accumulating logger warnings for experimental features
Browse files Browse the repository at this point in the history
  • Loading branch information
MaciejG604 committed Sep 5, 2023
1 parent d1436d4 commit 55f5be3
Show file tree
Hide file tree
Showing 13 changed files with 153 additions and 61 deletions.
2 changes: 2 additions & 0 deletions modules/build/src/main/scala/scala/build/CrossSources.scala
Original file line number Diff line number Diff line change
Expand Up @@ -235,6 +235,8 @@ object CrossSources {
distinctSources
}

logger.flushExperimentalWarnings

val scopedRequirements = preprocessedSources.flatMap(_.scopedRequirements)
val scopedRequirementsByRoot = scopedRequirements.groupBy(_.path.root)
def baseReqs(path: ScopePath): BuildRequirements = {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,12 +1,13 @@
package scala.build

import bloop.rifle.BloopRifleLogger
import org.scalajs.logging.{Logger => ScalaJsLogger}
import org.scalajs.logging.Logger as ScalaJsLogger

import java.io.PrintStream

import scala.build.errors.{BuildException, Diagnostic}
import scala.scalanative.{build => sn}
import scala.build.internals.FeatureType
import scala.scalanative.build as sn

class PersistentDiagnosticLogger(parent: Logger) extends Logger {
private val diagBuilder = List.newBuilder[Diagnostic]
Expand Down Expand Up @@ -39,4 +40,9 @@ class PersistentDiagnosticLogger(parent: Logger) extends Logger {
def compilerOutputStream: PrintStream = parent.compilerOutputStream

def verbosity: Int = parent.verbosity

def experimentalWarning(featureName: String, featureType: FeatureType): Unit =
parent.experimentalWarning(featureName, featureType)

def flushExperimentalWarnings: Unit = parent.flushExperimentalWarnings
}
Original file line number Diff line number Diff line change
Expand Up @@ -2,27 +2,34 @@ package scala.build.internal.util

import scala.build.input.ScalaCliInvokeData
import scala.build.internal.Constants
import scala.build.internals.FeatureType
import scala.build.preprocessing.directives.{DirectiveHandler, ScopedDirective}
import scala.cli.commands.{SpecificationLevel, tags}
import scala.cli.config.Key

object WarningMessages {
private val scalaCliGithubUrl = s"https://github.com/${Constants.ghOrg}/${Constants.ghName}"
private def experimentalFeatureUsed(featureName: String): String =
s"""$featureName is experimental.
|Please bear in mind that non-ideal user experience should be expected.

private val experimentalNote =
s"""Please bear in mind that non-ideal user experience should be expected.
|If you encounter any bugs or have feedback to share, make sure to reach out to the maintenance team at $scalaCliGithubUrl""".stripMargin
def experimentalDirectiveUsed(name: String): String =
experimentalFeatureUsed(s"The `$name` directive")
def experimentalFeaturesUsed(namesAndFeatureTypes: Seq[(String, FeatureType)]): String = {
val message = namesAndFeatureTypes match {
case Seq((name, featureType)) => s"The $name $featureType is experimental"
case namesAndTypes =>
val nl = System.lineSeparator()
val bulletPointList = namesAndTypes.map((name, fType) => s" - `$name` $fType")
.mkString(nl)
s"""Some utilized features are marked as experimental:
|$bulletPointList""".stripMargin
}
s"""$message
|$experimentalNote""".stripMargin
}

def experimentalSubcommandUsed(name: String): String =
experimentalFeatureUsed(s"The `$name` sub-command")

def experimentalOptionUsed(name: String): String =
experimentalFeatureUsed(s"The `$name` option")

def experimentalConfigKeyUsed(name: String): String =
experimentalFeatureUsed(s"The `$name` configuration key")
s"""The `$name` sub-command is experimental.
|$experimentalNote""".stripMargin

def rawValueNotWrittenToPublishFile(
rawValue: String,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ import scala.build.errors.{
}
import scala.build.input.ScalaCliInvokeData
import scala.build.internal.util.WarningMessages
import scala.build.internal.util.WarningMessages.experimentalDirectiveUsed
import scala.build.internals.FeatureType
import scala.build.options.{
BuildOptions,
BuildRequirements,
Expand Down Expand Up @@ -123,11 +123,12 @@ case class DirectivesPreprocessor(
if !allowRestrictedFeatures && (handler.isRestricted || handler.isExperimental) then
Left(DirectiveErrors(
::(WarningMessages.powerDirectiveUsedInSip(scopedDirective, handler), Nil),
// TODO: use positions from ExtractedDirectives to get the full directive underlined
DirectiveUtil.positions(scopedDirective.directive.values, path)
))
else
if handler.isExperimental && !shouldSuppressExperimentalFeatures then
logger.message(experimentalDirectiveUsed(scopedDirective.directive.toString))
logger.experimentalWarning(scopedDirective.directive.toString, FeatureType.Directive)
handler.handleValues(scopedDirective, logger)

val handlersMap = handlers
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,19 +3,19 @@ package scala.build.tests
import bloop.rifle.BloopRifleLogger
import com.eed3si9n.expecty.Expecty.expect
import coursier.cache.CacheLogger
import org.scalajs.logging.{Logger => ScalaJsLogger, NullLogger}
import org.scalajs.logging.{NullLogger, Logger as ScalaJsLogger}

import java.io.PrintStream

import scala.build.Ops._
import scala.build.Ops.*
import scala.build.errors.{BuildException, Diagnostic, Severity}
import scala.build.input.Inputs
import scala.build.internals.FeatureType
import scala.build.options.{
BuildOptions,
InternalOptions,
JavaOptions,
ScalacOpt,
ScalaOptions,
ScalacOpt,
Scope,
ShadowingSeq
}
Expand Down Expand Up @@ -61,6 +61,8 @@ class BuildProjectTests extends munit.FunSuite {

override def verbosity = ???

override def experimentalWarning(featureName: String, featureType: FeatureType): Unit = ???
override def flushExperimentalWarnings: Unit = ???
}

val bloopJavaPath = Position.Bloop("/home/empty/jvm/8/")
Expand Down
10 changes: 8 additions & 2 deletions modules/build/src/test/scala/scala/build/tests/TestLogger.scala
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,13 @@ package scala.build.tests
import bloop.rifle.BloopRifleLogger
import coursier.cache.CacheLogger
import coursier.cache.loggers.{FallbackRefreshDisplay, RefreshLogger}
import org.scalajs.logging.{Logger => ScalaJsLogger, NullLogger}
import org.scalajs.logging.{NullLogger, Logger as ScalaJsLogger}

import scala.build.errors.BuildException
import scala.build.Logger
import scala.scalanative.{build => sn}
import scala.scalanative.build as sn
import scala.build.errors.Diagnostic
import scala.build.internals.FeatureType

case class TestLogger(info: Boolean = true, debug: Boolean = false) extends Logger {

Expand Down Expand Up @@ -83,4 +84,9 @@ case class TestLogger(info: Boolean = true, debug: Boolean = false) extends Logg
if (debug) 2
else if (info) 0
else -1

override def experimentalWarning(featureName: String, featureType: FeatureType): Unit =
System.err.println(s"Experimental $featureType `$featureName` used")

override def flushExperimentalWarnings: Unit = ()
}
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import caseapp.core.{Arg, Error}
import scala.build.Logger
import scala.build.input.ScalaCliInvokeData
import scala.build.internal.util.WarningMessages
import scala.build.internals.FeatureType
import scala.cli.ScalaCli
import scala.cli.util.ArgHelpers.*

Expand Down Expand Up @@ -54,7 +55,7 @@ object RestrictedCommandsParser {
))
case (r @ Right(Some(_, arg: Arg, _)), passedOption :: _)
if arg.isExperimental && !shouldSuppressExperimentalWarnings =>
logger.message(WarningMessages.experimentalOptionUsed(passedOption))
logger.experimentalWarning(passedOption, FeatureType.Option)
r
case (other, _) =>
other
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import scala.build.errors.BuildException
import scala.build.input.{ScalaCliInvokeData, SubCommand}
import scala.build.internal.util.WarningMessages
import scala.build.internal.{Constants, Runner}
import scala.build.internals.FeatureType
import scala.build.options.{BuildOptions, ScalacOpt, Scope}
import scala.build.{Artifacts, Directories, Logger, Positioned, ReplArtifacts}
import scala.cli.commands.default.LegacyScalaOptions
Expand Down Expand Up @@ -346,6 +347,8 @@ abstract class ScalaCommand[T <: HasGlobalOptions](implicit myParser: Parser[T],
* start of running every [[ScalaCommand]].
*/
final override def run(options: T, remainingArgs: RemainingArgs): Unit = {
logger.flushExperimentalWarnings

CurrentParams.verbosity = options.global.logging.verbosity
if shouldExcludeInSip then
logger.error(WarningMessages.powerCommandUsedInSip(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import java.util.Base64
import scala.build.Ops.*
import scala.build.errors.{BuildException, CompositeBuildException, MalformedCliInputError}
import scala.build.internal.util.WarningMessages
import scala.build.internals.FeatureType
import scala.build.{Directories, Logger}
import scala.cli.ScalaCli.allowRestrictedFeatures
import scala.cli.commands.pgp.PgpScalaSigningOptions
Expand Down Expand Up @@ -115,7 +116,7 @@ object Config extends ScalaCommand[ConfigOptions] {
sys.exit(1)
case Some(entry) =>
if entry.isExperimental && !shouldSuppressExperimentalFeatureWarnings then
logger.message(WarningMessages.experimentalConfigKeyUsed(entry.fullName))
logger.experimentalWarning(entry.fullName, FeatureType.ConfigKey)
if (values.isEmpty)
if (options.unset) {
db.remove(entry)
Expand Down Expand Up @@ -292,6 +293,8 @@ object Config extends ScalaCommand[ConfigOptions] {
}
}
}

logger.flushExperimentalWarnings
}

/** Check whether to ask for an update depending on the provided key.
Expand Down
28 changes: 25 additions & 3 deletions modules/cli/src/main/scala/scala/cli/internal/CliLogger.scala
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ package scala.cli.internal

import bloop.rifle.BloopRifleLogger
import ch.epfl.scala.bsp4j.Location
import ch.epfl.scala.{bsp4j => b}
import ch.epfl.scala.bsp4j as b
import coursier.cache.CacheLogger
import coursier.cache.loggers.{FallbackRefreshDisplay, RefreshLogger}
import org.scalajs.logging.{Level => ScalaJsLevel, Logger => ScalaJsLogger, ScalaConsoleLogger}
Expand All @@ -12,10 +12,13 @@ import java.io.PrintStream
import scala.build.bsp.protocol.TextEdit
import scala.build.errors.{BuildException, CompositeBuildException, Diagnostic, Severity}
import scala.build.internal.CustomProgressBarRefreshDisplay
import scala.build.internal.util.WarningMessages
import scala.build.internals.FeatureType
import scala.build.options.ShadowingSeq
import scala.build.{ConsoleBloopBuildClient, Logger, Position}
import scala.collection.mutable
import scala.jdk.CollectionConverters._
import scala.scalanative.{build => sn}
import scala.jdk.CollectionConverters.*
import scala.scalanative.build as sn

class CliLogger(
val verbosity: Int,
Expand Down Expand Up @@ -131,6 +134,7 @@ class CliLogger(
if (verbosity >= 2)
printEx(ex, new mutable.HashMap)
def exit(ex: BuildException): Nothing =
flushExperimentalWarnings
if (verbosity < 0)
sys.exit(1)
else if (verbosity == 0) {
Expand Down Expand Up @@ -205,6 +209,24 @@ class CliLogger(

// Allow to disable that?
def compilerOutputStream = out

private var experimentalWarnings: Map[FeatureType, Set[String]] = Map()
def experimentalWarning(featureName: String, featureType: FeatureType): Unit = {
experimentalWarnings ++= experimentalWarnings.updatedWith(featureType) {
case None => Some(Set(featureName))
case Some(namesSet) => Some(namesSet + featureName)
}
}
def flushExperimentalWarnings: Unit = if (experimentalWarnings.nonEmpty) {
val messageStr = {
val namesAndTypes = for {
(featureType, names) <- experimentalWarnings.toSeq
name <- names
} yield name -> featureType
WarningMessages.experimentalFeaturesUsed(namesAndTypes)
}
message(messageStr)
}
}

object CliLogger {
Expand Down
10 changes: 10 additions & 0 deletions modules/core/src/main/scala/scala/build/Logger.scala
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import org.scalajs.logging.{Logger => ScalaJsLogger, NullLogger}
import java.io.{OutputStream, PrintStream}

import scala.build.errors.{BuildException, Diagnostic, Severity}
import scala.build.internals.FeatureType
import scala.scalanative.{build => sn}

trait Logger {
Expand Down Expand Up @@ -37,6 +38,12 @@ trait Logger {
def compilerOutputStream: PrintStream

def verbosity: Int

/** Since we have a lot of experimental warnings all over the build process, this method can be
* used to accumulate them for a better UX
*/
def experimentalWarning(featureName: String, featureType: FeatureType): Unit
def flushExperimentalWarnings: Unit
}

object Logger {
Expand Down Expand Up @@ -73,6 +80,9 @@ object Logger {
)

def verbosity: Int = -1

def experimentalWarning(featureUsed: String, featureType: FeatureType): Unit = ()
def flushExperimentalWarnings: Unit = ()
}
def nop: Logger = new Nop
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
package scala.build.internals

enum FeatureType(stringRepr: String) {
override def toString: String = stringRepr

case Option extends FeatureType("option")
case Directive extends FeatureType("directive")
case Subcommand extends FeatureType("sub-command")
case ConfigKey extends FeatureType("configuration key")
}
Loading

0 comments on commit 55f5be3

Please sign in to comment.