reduce backend IO load #5834
reduce backend IO load #5834
Conversation
|
will have a look at the test failure tonight |
|
The two failures are: |
|
@lrytz thanks for the diagnostic - will have a look Is the expectation that the compiler will delete whatever files to create the directories needed, and similarly delete directories to create files, when the names conflict. The test case is not specific (t5717), though it should be easy to fix to whatever is the expectation (either of the above seems dangerous though) Always wary about deleting trees :( I can fix this when I get a clarification on the expectation above For the other issue - I suspect that this is due to there being duplicate classes with the same name in the test - as I read macro-range both "Range" and "Test" are duplicated in the root package. Does this test compile all 3 files are they incrementally compiler in phases Sorry for my ignorance of this but haven't really looked at these tests and how then run |
|
Running tests is easy in sbt (https://github.com/scala/scala#using-the-sbt-build) About About In the example, it first compiles The third file contains the exact same definitions of |
|
Paulp's first fix for SI-5717 was destructive, but the use case of outputting to the current dir is not far-fetched. I typically have a scratch or tmp dir where I have misc test classes, and occasionally a text file that is unfortunately named and clashes with a package. Maybe there's a useful heuristic, such as specifying an output dir makes it trashable. |
|
@lrytz thanks for that detail - I will add a catch and some appropriate diagnostic. Probably Monday night when I am back I have add a fix for the macro-range issue in 8dd934a I think that this was caught by luck though. It was by chance that this test ( and this test only) wrote a slightly smaller file in the second pass ( not sure why) What is the naming convention of these tests? I was aware of how to run the tests from sbt (https://github.com/scala/scala#using-the-sbt-build), but sbt partest fails on windows, and the ++ syntax doesn't work to override the build dir (from cmd) |
If there's a ticket on https://github.com/scala/bug/issues, it's tNNNN, otherwise be there's none.. |
Ah! We have a nightly build on windows, and it's passing (https://scala-ci.typesafe.com/job/scala-2.12.x-integrate-windows/). However, this runs in cygwin, not cmd. @szeiger might be able to help out. Our README just says "Windows may work if you use Cygwin".. |
|
"Runs in Cygwin" is overstating it a bit. In Cygwin you get the sbt launcher bash script instead of the Windows cmd script but that's it. The JVM processes are not Cygwin-aware, so once you're in the JVM there shouldn't be any difference. The only exception I can think of is calling an external "diff" tool to show diffs in case of partest failures. If there's no POSIX-compatible I'd go looking at the sbt launcher and its configuration as possible sources of errors. AFAIR configuration of JVM parameters is done differently in the bash and cmd launchers, so this could explain different results (when the parameters that you expect are not actually used). |
|
I was just looking at partest to make it pass -Xlint and also scala/scala-partest#55 oh wait I guess scala/scala-partest#81 about diff order, and I noticed that it will use git diff or else difflib as available, so possibly under --update-check it should always use difflib for reproducibility. Edit: or I guess it always just logs the log output; the diff is just for human consumption, either diff suffices to pass/fail the test. I'll look again later when I have time, but that seems obvious. |
|
@som-snytt That's probably the right explanation. We already know from the problems leading to #5733 that @mkeskells' |
|
@som-snytt @szeiger I ran on a clean build from the start or last week after #5733 was merged |
d65c7ea to
7b32e78
Compare
|
@mkeskells #5733 wouldn't fix this but it's the same symptom. Executing |
|
@szeiger I preumed that it fixed git usage. I suppose this was just to get some minimal set running for the mkPack? |
7b32e78 to
3a0023a
Compare
|
all test finally pass - can squash, but will leave so the reviewers can see the changes, and squash when I get the nod |
There was a problem hiding this comment.
Thanks for the changes, they look good! Also thanks for the new tests.
I implemented a few cleanups here: rorygraves#2. We're trying to keep the formatting (more or less) consistent in this repo, so it'd be nice if you could use t: T (not t:T or t :T). Some other details:
- No empty line before the first / after the last expression in a block (class, method)
- Empty line between methods, classes
- No space in
foo(arg - Space in
if (..),C {,foo = {
You can squash the changes into one commit.
Finally, the comment #5832 (comment) about RunnableInPhase also applies here.
test/files/run/t5717.scala
Outdated
| @@ -13,9 +13,19 @@ object Test extends StoreReporterDirectTest { | |||
| // Let the compiler tell us output dir | |||
| // val sc = newCompiler("-cp", classpath, "-d", testOutput.path) | |||
| // val out = sc.settings.outputDirs.getSingleOutput.get | |||
| // TODO check the reported errors | |||
There was a problem hiding this comment.
for this, just rebase this PR on current 2.12.x, #5835 is merged
|
@mkeskells That fix was only for using git to get the commit SHA and date, not for any other uses (but |
|
is there a intellij formatter for the code style anywhere |
|
I don't think so, also we don't have our conventions written down, I just mentioned a few things that people seem to follow here. Things are not too strict either. It would be nice to adopt scalfmt, but require a bit of work.. |
|
This patch looks good to me now, I think you can squash everything into one. I'd also remove the first commit ( |
af1c05a to
ece7fe2
Compare
ece7fe2 to
ba8f25e
Compare
cache created directory, faster IO and parsing of dir structure dont use AbstractFile for files that you want to create
ba8f25e to
c293ac1
Compare
|
Squashed and removed the first commit (profiler enabler), and rebased as requested The rebase changed so that t5717 now has validated output so I have changed the expectation to match the modified test now |
|
will revist this after @lrytz has completed his refactor |
cache created directory,
faster IO and parsing of dir structure
dont use AbstractFile for files that you want to create
performance results - looks good