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

Revert "Update directory-watcher to 0.8.3" #1126

Merged
merged 3 commits into from Dec 3, 2019

Conversation

@olafurpg
Copy link
Member

olafurpg commented Dec 2, 2019

The PR #1108 introduced an uncaught regression on macOS.

Exception in thread "pool-2-thread-2" java.lang.NoSuchFieldError: SIZE
	at com.zaxxer.nuprocess.osx.OsxProcess.createPosixSpawnFileActions(OsxProcess.java:191)
	at com.zaxxer.nuprocess.osx.OsxProcess.createPosixPipes(OsxProcess.java:150)
	at com.zaxxer.nuprocess.osx.OsxProcess.start(OsxProcess.java:58)
	at com.zaxxer.nuprocess.osx.OsxProcessFactory.createProcess(OsxProcessFactory.java:34)
	at com.zaxxer.nuprocess.NuProcessBuilder.start(NuProcessBuilder.java:266)
	at scala.meta.internal.process.SystemProcess$.run(SystemProcess.scala:27)
	at scala.meta.internal.pantsbuild.Filemap$.fromPants(Filemap.scala:40)
	at scala.meta.internal.pantsbuild.BloopPants$.$anonfun$bloopInstall$1(BloopPants.scala:133)
	at scala.runtime.java8.JFunction0$mcI$sp.apply(JFunction0$mcI$sp.java:23)
	at scala.meta.internal.pantsbuild.BloopPants$.interruptedTry(BloopPants.scala:109)

Now, we run tests on macOS to prevent further regressions.

olafurpg added 2 commits Dec 2, 2019
The PR #1108 introduced an
uncaught regression on macOS. Now, we run tests on macOS to prevent
further regressions.
@olafurpg olafurpg force-pushed the olafurpg:macos-revert branch from 809da89 to a1dde88 Dec 2, 2019
@olafurpg

This comment has been minimized.

Copy link
Member Author

olafurpg commented Dec 2, 2019

I remember having manually tried upgrading directory-watcher but hitting on the same issue 😟 I'm tempted to try and use another library instead like https://github.com/lihaoyi/os-lib/

@olafurpg olafurpg requested a review from tgodzik Dec 2, 2019
@olafurpg

This comment has been minimized.

Copy link
Member Author

olafurpg commented Dec 2, 2019

Looks like we have some work to do to fix failing tests on macOS 😭

[error] Failed tests:
[error] 	tests.ImplementationSuite
[error] 	tests.ReferenceLspSuite
[error] 	tests.RenameLspSuite
Copy link
Collaborator

tgodzik left a comment

Awesome that we can enable the test on mac! Thanks!

It seems that a lot of the new tests are failing there, are you able to run them locally?

@tgodzik

This comment has been minimized.

Copy link
Collaborator

tgodzik commented Dec 3, 2019

I checked on a Mac and it seems there is a race - the tests are are run before semanticDB files are added to Bloom filters - maybe we can register in the directory watcher to await also the semanticDB files in TestServer.

@olafurpg

This comment has been minimized.

Copy link
Member Author

olafurpg commented Dec 3, 2019

@tgodzik thank you for checking it out! That sounds plausible. I'll see we can do.

@olafurpg

This comment has been minimized.

Copy link
Member Author

olafurpg commented Dec 3, 2019

@tgodzik I can confirm that it looks indeed like a race. Adding a Thread.sleep() seems to make the tests pass 🤔

The core of the problem is that the tests are relying on the file watcher notifications to trigger for the indexer to process SemanticDB files.

Previously, the tests could fail due to delayed file watching
notifications that triggered SemanticDB indexing. Now, we manually
trigger SemanticDB indexing after `didCompile` notifications to make the
tests run more reliably.
@olafurpg

This comment has been minimized.

Copy link
Member Author

olafurpg commented Dec 3, 2019

I believe the latest commit fixes the macOS issues in a not too hacky way. Instead of relying on file watching notifications when running tests, we now walk the META-INF/semanticdb directory after "compile finished" notifications and manually trigger file watching notifications for SemanticDB files.

Given the issues we have experience with file watching, I am tempted to try https://github.com/lihaoyi/os-lib/#oswatchwatch to see if that implementation is more reliable than our current file watching library.

@olafurpg olafurpg requested a review from tgodzik Dec 3, 2019
@olafurpg

This comment has been minimized.

Copy link
Member Author

olafurpg commented Dec 3, 2019

CI is green! 🎉

@tgodzik

This comment has been minimized.

Copy link
Collaborator

tgodzik commented Dec 3, 2019

CI is green!

cool! 😍

@tgodzik
tgodzik approved these changes Dec 3, 2019
Copy link
Collaborator

tgodzik left a comment

LGTM I don't think there is any other way to fix it properly.

I will try to update the directory watcher to the newest version later on and it might be possible reenable some tests on windows.

@tgodzik tgodzik merged commit 2d4b291 into scalameta:master Dec 3, 2019
10 checks passed
10 checks passed
ubuntu-latest tests
Details
windows-latest tests
Details
macOS-latest tests
Details
Sbt integration
Details
Maven integration
Details
Gradle integration
Details
Mill integration
Details
Slow tests
Details
Scala cross tests
Details
Scalafmt/Scalacheck/Docs
Details
@olafurpg olafurpg deleted the olafurpg:macos-revert branch Dec 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.