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

interop: enable preserveArgvZero #84

Merged
merged 1 commit into from
Apr 27, 2022
Merged

interop: enable preserveArgvZero #84

merged 1 commit into from
Apr 27, 2022

Conversation

K900
Copy link
Contributor

@K900 K900 commented Apr 21, 2022

Fixes interop with latest WSL versions. Would appreciate some testing on pre-Store versions of WSL.

@K900
Copy link
Contributor Author

K900 commented Apr 21, 2022

As a simple way to test this, enable wsl.interop.register, then call /mnt/c/Windows/notepad.exe C:\test.txt. If it works correctly, that means the change works.

@K900
Copy link
Contributor Author

K900 commented Apr 21, 2022

Update: great. This change breaks stable and fixes Preview. Will need a thonk.

@K900 K900 marked this pull request as draft April 21, 2022 21:12
@K900
Copy link
Contributor Author

K900 commented Apr 21, 2022

Update: it seems to be impossible to actually detect the WSL version from inside the VM. This is just great.

@nzbr
Copy link
Member

nzbr commented Apr 21, 2022

As of now, it is still working for me like before, however I am only in the Release Preview ring on this machine. Even if we could detect the WSL version, we'd need to do that at build time (or register WSLInterop using a custom systemd service instead of systemd-binfmt, but that'd be more of a workaround than a solution)

@K900
Copy link
Contributor Author

K900 commented Apr 21, 2022

You need WSL Preview from the Microsoft Store to get the new behavior. Also, we can register a shim that just reexecs /init with the right args, but that still needs to somehow know the version...

@nzbr nzbr added the bug Something isn't working label Apr 21, 2022
@K900
Copy link
Contributor Author

K900 commented Apr 22, 2022

Let's just ask upstream: microsoft/WSL#8315

@K900
Copy link
Contributor Author

K900 commented Apr 22, 2022

Also, here's the upstream update that contains the change: https://github.com/microsoft/WSL/releases/tag/0.58.0

@K900
Copy link
Contributor Author

K900 commented Apr 22, 2022

So turns out upstream has a solution: torvalds/linux@2347961. Yay!

Except it's only in kernel 5.12 and up, and WSL2 ships 5.10. Boo.

So I guess we have to figure something out still, or coerce upstream into fixing this correctly?

@nzbr
Copy link
Member

nzbr commented Apr 22, 2022

This works somehow (with the current code, on the WSL preview)

image

@K900
Copy link
Contributor Author

K900 commented Apr 22, 2022

That does work, but it relies on interop being enabled again. If we just assume interop is enabled in wsl.conf, we can just read the flags from sysfs on startup.

@nzbr
Copy link
Member

nzbr commented Apr 22, 2022

This is with wsl.interop.register enabled. We could move the register process to a custom systemd service, then register using the old method. If we can run powershell.exe -C exit, we leave it as is, otherwise we re-register with preserveArgvZero enabled

@K900
Copy link
Contributor Author

K900 commented Apr 22, 2022

That's not the problem, the problem is getting the PATH on the VM side. Though I guess we could use a custom executable in the store as a test...

@nzbr
Copy link
Member

nzbr commented Apr 22, 2022

How ist the PATH a problem? The path seems to be set correctly in WSL Preview. Also, the path to powershell.exe is well-known (${config.wsl.automountPath}/c/Windows/System32/WindowsPowerShell/v1.0)

@K900
Copy link
Contributor Author

K900 commented Apr 22, 2022

PATH is only set if interop is enabled, which it might not be, and Windows doesn't have to live on C:. I think we can make a custom executable work though - it can totally run things stored on the VM side over 9p.

@nzbr
Copy link
Member

nzbr commented Apr 22, 2022

I haven't considered that...

I know that it can, that's what I did in #83. We could use a small binary in a language that can be cross-compiled on Linux for Windows (I think go would work)

@K900
Copy link
Contributor Author

K900 commented Apr 23, 2022

So I've just made a very simple Rust shim that prints argv and exits. Here's what it's getting when invoked in different ways.

  • WSL 0.58+

    • /init shim.exe -> does not start
    • /init shim.exe shim.exe -> ["shim.exe"]
    • ./shim.exe, flags: PF -> ["shim.exe"]
    • ./shim.exe, flags: F -> does not start
    • ./shim.exe shim.exe, flags: PF -> ["shim.exe", "shim.exe"]
    • ./shim.exe shim.exe, flags: F -> ["shim.exe"]
  • WSL <0.58

    • /init shim.exe -> ["shim.exe"]
    • /init shim.exe shim.exe -> ["shim.exe", "shim.exe"]
    • ./shim.exe, flags: PF -> ["shim.exe", "./shim.exe"]
    • ./shim.exe, flags: F -> ["shim.exe"]
    • ./shim.exe shim.exe, flags: PF -> ["shim.exe", "./shim.exe", "shim.exe"]
    • ./shim.exe shim.exe, flags: F -> ["shim.exe", "shim.exe"]

This led me to a more interesting discovery: /init fake.exe exits with exit code 1 if it can't parse the command line, and exit code 255 if it can't start the application, meaning we can actually use the exit code to know the right way to invoke it.

@K900
Copy link
Contributor Author

K900 commented Apr 23, 2022

So, this is very ugly and we really need upstream's help here, but it might work for now?

@K900
Copy link
Contributor Author

K900 commented Apr 23, 2022

Pushed a shorter version with a better comment.

@nzbr
Copy link
Member

nzbr commented Apr 23, 2022

This looks at least usable for the time being. Once WSL 0.58 (or later) gets included in a stable windows build, the hack could be moved behind an option (something like wsl.interop.argvCompat) and a hint in the release notes of the release after that happens

@K900
Copy link
Contributor Author

K900 commented Apr 24, 2022

Another option would be to add a tristate null (autodetect)/true/false option for this, just to save the overhead of the extra /init call. I'm not sure it's worth it though...

@K900 K900 marked this pull request as ready for review April 26, 2022 05:57
@K900
Copy link
Contributor Author

K900 commented Apr 26, 2022

I guess this is fundamentally a hack, so there's no real reason to attempt to unhack it.

@sielicki
Copy link

+1 from me, I think this is a good solution. Let's not get too caught up in whether things are "hacks", WSL is moving too fast and these things will be irrelevant in a few hundred days.

@K900
Copy link
Contributor Author

K900 commented Apr 27, 2022

I sincerely hope so.

@sielicki
Copy link

So turns out upstream has a solution: torvalds/linux@2347961. Yay!

Except it's only in kernel 5.12 and up, and WSL2 ships 5.10. Boo.

So I guess we have to figure something out still, or coerce upstream into fixing this correctly?

they've indicated that they're willing to backport anything that has landed in master if it's relevant, so it might be worth sending an email directly. Their issue tracker is a mess, just the way it goes when they have as many new-to-linux users as they have.

@K900
Copy link
Contributor Author

K900 commented Apr 27, 2022

The problem is that even if they backport it, we already know how the new version behaves anyway.

@K900
Copy link
Contributor Author

K900 commented Apr 27, 2022

As in the backport will ship in a future release that already has consistent behavior.

@nzbr
Copy link
Member

nzbr commented Apr 27, 2022

image

I did a little benchmark of the autodetect wrapper (starting a small binary that just exits immediately, repeated 10000 times, times in seconds). It effectively doubles the invocation time. That probably won't be noticeable, but to ensure backward compatibility, we should probably leave the hack in here as an option even when the new WSL version becomes stable. I like the idea with the tristate option. For the moment the default will be null (autodetect) and later we change it to false by default

@K900
Copy link
Contributor Author

K900 commented Apr 27, 2022

I'd rather remove the hack, default to preserveArgvZero, and let people on old WSLs use the wrapper. Also, if this ever becomes performance critical, we can always move to detecting the behavior in an activation script or whatever.

@K900
Copy link
Contributor Author

K900 commented Apr 27, 2022

The more I think about it the more I feel like this should be an option still, and we can just default it to true when the update ships.

@K900 K900 marked this pull request as draft April 27, 2022 17:51
@K900
Copy link
Contributor Author

K900 commented Apr 27, 2022

I'll set this to WIP and try to make the option tomorrow.

@nzbr
Copy link
Member

nzbr commented Apr 27, 2022

The legacy registration should definitely be an option for compatibility reasons. The only question is whether or not we should keep Auto-detection as an option (which I think could be useful in some cases, for example of you have a config you want to deploy to multiple machines running different versions of WSL)

@K900
Copy link
Contributor Author

K900 commented Apr 27, 2022

I was thinking about just putting the hack in now, then making it optional when the update ships. But that doesn't really make sense, does it...

@nzbr
Copy link
Member

nzbr commented Apr 27, 2022

Why not, I mean with the autodetection enabled it works for now. I could merge now and you open a new PR where you add the option tomorrow or whenever you have time for it. When the update ships, we just have to change the default

@K900
Copy link
Contributor Author

K900 commented Apr 27, 2022

I'm trying to add the option now...

@K900
Copy link
Contributor Author

K900 commented Apr 27, 2022

Still need to test this with different versions, give me a bit.

Fixes interop with latest WSL versions
@K900 K900 marked this pull request as ready for review April 27, 2022 18:12
@K900
Copy link
Contributor Author

K900 commented Apr 27, 2022

OK, tested on my end, seems good.

@nzbr nzbr merged commit 506d46f into nix-community:main Apr 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants