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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Windows client-server fixes #1230

Merged
merged 5 commits into from
Mar 24, 2021
Merged

Conversation

sake92
Copy link
Contributor

@sake92 sake92 commented Mar 22, 2021

Only one thing in ClientServerTests doesn't work currently.
Maybe someone will find a fix in the future... 馃槃

I tried many things, even reimplementing the whole named pipes thing with JNA...
But it boils down to read/write throwing an exception instead of returning -1...
Since the read is done infinitely in a separate thread, it doesn't know when the output from server is done.
So when a connection breaks, we should just ignore it, and not System.exit(1); as currently.

Copy link
Member

@lefou lefou left a comment

Choose a reason for hiding this comment

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

Looks good to me. I added a question below. Does this PR really fix the server mode on Window, so that you no longer need the --no-server settings?

main/src/main/MillServerMain.scala Outdated Show resolved Hide resolved
@sake92
Copy link
Contributor Author

sake92 commented Mar 24, 2021

Looks good to me. I added a question below. Does this PR really fix the server mode on Window, so that you no longer need the --no-server settings?

It works 99% of the time.
It throws this exception when you run mill on another project:

Exception in thread "main" java.io.IOException: Could not create lock for \\.\pipe\mill.ghrxuj6QppDw=-1_lock, error 5
        at org.scalasbt.ipcsocket.Win32NamedPipeServerSocket.<init>(Win32NamedPipeServerSocket.java:104)

That's because there should only be one locked pipe.
But we give it the same pipe name, based on JDK we're running...
Not sure how to improve on this one?

But after that it works fine. 馃槃
This PR is a big improvement on status quo IMHO.

@sake92
Copy link
Contributor Author

sake92 commented Mar 24, 2021

@lefou I think we can take inspiration from Gradle deamon docs.
It spawns a new deamon per JDK+(JVM properties).
That seems reasonable because not all projects use same props, maybe even in tests you want different props?

Then, it doesn't seem logical to lock on processLock/serverLock because JDK/daemon should be global for all its clients, not per-project... (thus the exception above)

@lefou
Copy link
Member

lefou commented Mar 24, 2021

Thanks for this PR. I'll merge it. Nice to have mill server support on Windows.

@sake92, regarding the daemon: The mill daemon does not take multiple clients at once. Also I think we don't abstract the current working directory away in all cases, esp. when spawning sub-processes. So for having a shared daemon there is some more work to do...

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.

None yet

2 participants