-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Improve and document the Driver#process API, fix partest logging #1052
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
Conversation
* | ||
* This overload is provided for compatibility reasons: the | ||
* `RawCompiler` of sbt expect this method to exist and calls | ||
* it using reflection. Keeping it means that we can change |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
expects
Otherwise LGTM |
c04b38d
to
f9d22e5
Compare
@adriaanm, @SethTisue : any idea why Jenkins happily reports "success" when a job explodes with |
Nevermind, this seems to be sbt's fault, not Jenkins', I opened sbt/sbt#2442 |
/rebuild |
- Removed "-Xprint-types", it is only rarely needed and makes it very hard to read trees, enable it yourself if you need it. - Removed "-Ylog:<some", this does not seem to have any effect currently. - Removed "-pagewidth 90", this is overloaded in tests.scala to 160 anyway.
Also CompilerTest no longer runs the compiler with the context DottyTest#ctx. Previously, we got away with this because Compiler#process ignored it and created a new Context, but this commit fixes this, and it is now very important that we use a different context for every test we compile. Since DottyTest#ctx was the only part of DottyTest we used, CompilerTest no longer extends DottyTest to make sure that we do not use it accidentally. If we want to use DottyTest as a base class for tests again, we will have to remove its implicit Context field first. Also do not try to initialize the definitions in the context used by partest, this is not necessary.
With this commit and the previous one, partest should finally correctly log the output of the compiler and display it at the end of its execution if compilation failed. Also make sure the initial Reporter in a Context is _not_ a ThrowingReporter. Previously, the initial Reporter was always overriden in Compiler by rootReporter so it had no effect, but this is not the case anymore, and the negative junit tests fail with the ThrowingReporter since they throw an exception instead of exiting with a certain number of errors.
- Document the entry points - It is now possible to set a custom reporter without using a custom context - Use `null` for optional arguments to make it easier to run the compiler using reflection or from Java. - DPDirectCompiler does not use a custom context anymore
Previously, we could set compilerCallback on non-fresh contexts, but there is no reason that this should be allowed, and this is not done anymore in the code since the last commit.
f9d22e5
to
a33d517
Compare
/rebuild |
OK, tests now pass as expected, the missing ingredient was that |
Improve and document the Driver#process API, fix partest logging
I would prefer to be able to see the failure without needing to download and extract archive. |
If the compilation actually failed, the compilation logs will be displayed at the end of the partest run. |
Review by @DarkDimius or @odersky , this was very boring to do but very much needed.
Note that this means that when running the partest job, the compiler output will not be available just by scrolling through the giant log, for tests where compilation fails it will be at the bottom, for other tests (including tests which fail at runtime and not at compilation time), it will only be available in the *.clog files which can be retrieved by clicking on "Build Artifacts" on the Jenkins page for a job. If you're displeased with this, we could log it in the giant log too.