Skip to content
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

Avoid calling sbt deprecated methods, stop using log4j #2087

Merged
merged 2 commits into from
Oct 25, 2022

Conversation

lolgab
Copy link
Member

@lolgab lolgab commented Oct 25, 2022

This stops using log4j sbt logger in favor of the global logger, which should not log warnings around SecurityManager.

This is the log I'm talking about:

[#1] 2022-10-25 14:45:57,759 pool-1-thread-2 ERROR Could not register mbeans java.security.AccessControlException: access denied ("javax.management.MBeanTrustPermission" "register")
[#1]    at java.base/java.security.AccessControlContext.checkPermission(AccessControlContext.java:472)
[#1]    at java.base/java.lang.SecurityManager.checkPermission(SecurityManager.java:358)
[#1]    at java.management/com.sun.jmx.interceptor.DefaultMBeanServerInterceptor.checkMBeanTrustPermission(DefaultMBeanServerInterceptor.java:1805)
[#1]    at java.management/com.sun.jmx.interceptor.DefaultMBeanServerInterceptor.registerMBean(DefaultMBeanServerInterceptor.java:318)
[#1]    at java.management/com.sun.jmx.mbeanserver.JmxMBeanServer.registerMBean(JmxMBeanServer.java:522)
[#1]    at org.apache.logging.log4j.core.jmx.Server.register(Server.java:400)
[#1]    at org.apache.logging.log4j.core.jmx.Server.reregisterMBeansAfterReconfigure(Server.java:168)
[#1]    at org.apache.logging.log4j.core.jmx.Server.reregisterMBeansAfterReconfigure(Server.java:141)
[#1]    at org.apache.logging.log4j.core.LoggerContext.setConfiguration(LoggerContext.java:632)
[#1]    at org.apache.logging.log4j.core.LoggerContext.start(LoggerContext.java:285)
[#1]    at org.apache.logging.log4j.core.impl.Log4jContextFactory.getContext(Log4jContextFactory.java:209)
[#1]    at org.apache.logging.log4j.core.config.Configurator.initialize(Configurator.java:244)
[#1]    at org.apache.logging.log4j.core.config.Configurator.initialize(Configurator.java:220)
[#1]    at sbt.util.LogExchange.init(LogExchange.scala:151)
[#1]    at sbt.util.LogExchange.context$lzycompute(LogExchange.scala:24)
[#1]    at sbt.util.LogExchange.context(LogExchange.scala:24)
[#1]    at sbt.util.LogExchange.dummyLayout$lzycompute(LogExchange.scala:104)
[#1]    at sbt.util.LogExchange.dummyLayout(LogExchange.scala:103)
[#1]    at sbt.internal.util.Log4JConsoleAppender.<init>(ConsoleAppender.scala:566)
[#1]    at sbt.internal.util.ConsoleAppender.toLog4J$lzycompute(ConsoleAppender.scala:347)
[#1]    at sbt.internal.util.ConsoleAppender.toLog4J(ConsoleAppender.scala:342)
[#1]    at sbt.util.LogExchange.$anonfun$bindLoggerAppenders$2(LogExchange.scala:56)
[#1]    at scala.collection.immutable.List.map(List.scala:246)
[#1]    at scala.collection.immutable.List.map(List.scala:79)
[#1]    at sbt.util.LogExchange.bindLoggerAppenders(LogExchange.scala:56)
[#1]    at mill.scalalib.worker.ZincWorkerImpl.compileInternal(ZincWorkerImpl.scala:453)
[#1]    at mill.scalalib.worker.ZincWorkerImpl.$anonfun$compileMixed0$1(ZincWorkerImpl.scala:340)
[#1]    at mill.api.FixSizedCache.withCachedValue(FixSizedCache.scala:66)
[#1]    at mill.scalalib.worker.ZincWorkerImpl.withCompilers(ZincWorkerImpl.scala:422)
[#1]    at mill.scalalib.worker.ZincWorkerImpl.compileMixed0(ZincWorkerImpl.scala:339)
[#1]    at mill.scalalib.worker.ZincWorkerImpl.compileMixed(ZincWorkerImpl.scala:307)
[#1]    at mill.scalalib.ScalaModule.$anonfun$compile$2(ScalaModule.scala:198)
[#1]    at mill.define.Task$TraverseCtx.evaluate(Task.scala:380)
[#1]    at mill.eval.Evaluator.$anonfun$evaluateGroup$13(Evaluator.scala:627)
[#1]    at scala.util.DynamicVariable.withValue(DynamicVariable.scala:59)
[#1]    at scala.Console$.withErr(Console.scala:193)
[#1]    at mill.eval.Evaluator.$anonfun$evaluateGroup$12(Evaluator.scala:627)
[#1]    at scala.util.DynamicVariable.withValue(DynamicVariable.scala:59)
[#1]    at scala.Console$.withOut(Console.scala:164)
[#1]    at mill.eval.Evaluator.$anonfun$evaluateGroup$11(Evaluator.scala:626)
[#1]    at scala.util.DynamicVariable.withValue(DynamicVariable.scala:59)
[#1]    at scala.Console$.withIn(Console.scala:227)
[#1]    at mill.eval.Evaluator.$anonfun$evaluateGroup$8(Evaluator.scala:625)
[#1]    at mill.eval.Evaluator.$anonfun$evaluateGroup$8$adapted(Evaluator.scala:586)
[#1]    at scala.collection.immutable.Vector.foreach(Vector.scala:1856)
[#1]    at mill.eval.Evaluator.evaluateGroup(Evaluator.scala:586)
[#1]    at mill.eval.Evaluator.$anonfun$evaluateGroupCached$21(Evaluator.scala:478)
[#1]    at scala.util.DynamicVariable.withValue(DynamicVariable.scala:59)
[#1]    at mill.eval.Evaluator.evaluateGroupCached(Evaluator.scala:469)
[#1]    at mill.eval.Evaluator.$anonfun$parallelEvaluate$2(Evaluator.scala:298)
[#1]    at scala.concurrent.impl.Promise$Transformation.run(Promise.scala:467)
[#1]    at java.base/java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:515)
[#1]    at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:264)
[#1]    at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128)
[#1]    at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628)
[#1]    at java.base/java.lang.Thread.run(Thread.java:829)
[#1] 

The method we were calling was converting our appender into a log4j XAppender and then wrapping it in a ConsoleAppender.

  @deprecated("Use LoggerContext to bind appenders", "1.4.0")
  def bindLoggerAppenders(
      loggerName: String,
      appenders: List[(XAppender, Level.Value)]
  ): Unit = {
    appenders.foreach {
      case (a, l) =>
        LoggerContext.globalContext
          .addAppender(loggerName, new ConsoleAppenderFromLog4J(loggerName, a) -> l)
    }
  }
  @deprecated("Use LoggerContext to bind appenders", "1.4.0")
  def bindLoggerAppenders(
      loggerName: String,
      appenders: Seq[(Appender, Level.Value)]
  ): Unit = bindLoggerAppenders(loggerName, appenders.map { case (a, l) => a.toLog4J -> l }.toList)

Since we already create a ConsoleAppender we can avoid all these conversions and bypass log4j altogether.

This stops using log4j sbt logger in favor of the global
logger, which should not log warnings around SecurityManager
@lolgab lolgab marked this pull request as ready for review October 25, 2022 12:52
@lolgab lolgab requested a review from lefou October 25, 2022 12:52
Copy link
Member

@lefou lefou left a comment

Choose a reason for hiding this comment

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

This really feels a bit hacky. I opened an issue in the past, but instead of removing the private[sbt] scope, they instead removed the @deprecation of the used methods.

Here is the issue in sbt:

Also, from looking in the current implementation, the use of Log4j isn't completely disabled, it's only avoided. I think, there are even deeper issues in sbt and the way they use Log4j, as it seems to me, they somehow post-process log messages without sanitizing them.

Beside that, we are probably better to use the newer (hidden) API to create a logger anyways. It just doesn't feel ideal. We should ask sbt devs again, whether they can make this specific API public.

Copy link
Member

@lefou lefou left a comment

Choose a reason for hiding this comment

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

Looks as good as it can, I guess.

@lefou
Copy link
Member

lefou commented Oct 25, 2022

Huh, what's going on here? I pushed a commit to your branch, but this PR isn't updated. Hmm.

lolgab@f587d41

@lolgab
Copy link
Member Author

lolgab commented Oct 25, 2022

@lefou I can try to close the PR and open a new one.

@lefou lefou closed this Oct 25, 2022
@lefou lefou reopened this Oct 25, 2022
@lefou
Copy link
Member

lefou commented Oct 25, 2022

@lolgab I did it in-place. It helped.

@lefou lefou merged commit 200079c into com-lihaoyi:main Oct 25, 2022
@lefou
Copy link
Member

lefou commented Oct 25, 2022

Thank you!

@lefou lefou added this to the after 0.10.8 milestone Oct 25, 2022
@lolgab lolgab deleted the fix-sbt-deprecations-zinc branch October 25, 2022 14:17
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.

2 participants