Skip to content

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

Merged
merged 8 commits into from
Feb 4, 2016

Conversation

smarter
Copy link
Member

@smarter smarter commented Feb 4, 2016

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.

*
* 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
Copy link
Contributor

Choose a reason for hiding this comment

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

expects

@odersky
Copy link
Contributor

odersky commented Feb 4, 2016

Otherwise LGTM

@smarter smarter force-pushed the fix/driver-api branch 3 times, most recently from c04b38d to f9d22e5 Compare February 4, 2016 20:59
@smarter
Copy link
Member Author

smarter commented Feb 4, 2016

@adriaanm, @SethTisue : any idea why Jenkins happily reports "success" when a job explodes with OutOfMemoryException ? https://scala-ci.typesafe.com/job/dotty-master-validate-junit/1000/console

@smarter
Copy link
Member Author

smarter commented Feb 4, 2016

Nevermind, this seems to be sbt's fault, not Jenkins', I opened sbt/sbt#2442

@smarter
Copy link
Member Author

smarter commented Feb 4, 2016

/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.
@smarter smarter closed this Feb 4, 2016
@smarter smarter reopened this Feb 4, 2016
@smarter
Copy link
Member Author

smarter commented Feb 4, 2016

/rebuild

@smarter
Copy link
Member Author

smarter commented Feb 4, 2016

OK, tests now pass as expected, the missing ingredient was that CompilerTest reused the ctx field from DottyTest for every test, leading to weird errors, to prevent this CompilerTest no longer extends DottyTest. Ideally, someone should cleanup DottyTest and all the non-junit tests, see smarter@73fad44 for details.

smarter added a commit that referenced this pull request Feb 4, 2016
Improve and document the Driver#process API, fix partest logging
@smarter smarter merged commit 9d8c92d into scala:master Feb 4, 2016
@DarkDimius
Copy link
Contributor

If you're displeased with this, we could log it in the giant log too.

I would prefer to be able to see the failure without needing to download and extract archive.

@smarter
Copy link
Member Author

smarter commented Feb 4, 2016

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.

odersky added a commit to dotty-staging/dotty that referenced this pull request Mar 7, 2016
odersky referenced this pull request Mar 8, 2016
Allow successive opening comments.
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.

3 participants