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

Fix redirection of stdout stream to stderr when using show #2689

Merged
merged 11 commits into from Aug 8, 2023

Conversation

lihaoyi
Copy link
Member

@lihaoyi lihaoyi commented Aug 7, 2023

Fixes #2680

When you use show, we want to ensure that nothing goes to stdout except the JSON result of the task shown, so it can easily be piped and parsed and handled by external programs. This was likely broken in #2428.

The basic problem is that PrintLogger and PrefixLogger are different types, with only PrintLogger supporting withOutputStream but it quickly gets wrapped in PrefixLogger when passed around inside Mill, meaning MainModule.show0 sees that it's not an instance of PrintLogger and skips with withOutputStream.

This fix in this PR is to move withOutputStream to ColorLogger, and implement it in PrefixLogger as well.

Tested manually by via the patch and command below. On main, log.txt contains the COMPILING line in addition to the JSON blob (the bug above), and most of the Scala compiler output just disappears (which is also a bug!), and most of stdout goes to stderr (another bug!). On this PR, the Scala compiler output and COMPILING are both properly shown in the console, while the log.txt file contains only the JSON dictionary {"analysisFile": "...", "classes": "..."} and nothing else

--- a/example/basic/1-simple-scala/build.sc
+++ b/example/basic/1-simple-scala/build.sc
@@ -8,6 +8,11 @@ object foo extends RootModule with ScalaModule {
     ivy"com.lihaoyi::mainargs:0.4.0"
   )

+  def compile = T{
+    println("COMPILING")
+    super.compile()
+  }
+
   object test extends ScalaTests {
     def ivyDeps = Agg(ivy"com.lihaoyi::utest:0.7.11")
     def testFramework = "utest.runner.Framework"
echo // >> example/basic/1-simple-scala/src/Foo.scala && ./mill -i dev.run example/basic/1-simple-scala -i show compile > log.txt

Also updated the unit tests in mill.main.MainModuleTests to assert separately on the contents of stdout and stderr after running show, to ensure that exactly the right things end up in each place. mill.integration.WatchSourceInputTests also needed to be updated to properly assert on the expected stdout/stderr when show is given

There's probably more cleanup we can do w.r.t. ColorLogger/PrintLogger/PrefixLogger, but this PR just fixes the immediate problems for now and leaves better structuring of the Logger class hierarchy for later

@lihaoyi lihaoyi requested review from lefou and lolgab August 7, 2023 14:09
@lihaoyi
Copy link
Member Author

lihaoyi commented Aug 8, 2023

The failures in mill.integration.WatchSourceInputTests look like flakes, and these tests are known to be flaky on windows, but they seem pretty stubborn this time. I'm going to disable them on Windows

@lihaoyi lihaoyi merged commit 48b28e4 into com-lihaoyi:main Aug 8, 2023
37 checks passed
@lefou lefou added this to the 0.11.2 milestone Aug 9, 2023
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.

printlns interfere with show
2 participants