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

Sys.detectwsl() #36425

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Sys.detectwsl() #36425

wants to merge 1 commit into from

Conversation

mgautam98
Copy link
Contributor

Tries to solve #36354

Since it will be a runtime function I did it with Julia itself.

I looked into libuv docs and there is no wrapper function to get proc-version since Its too system specific and libuv abstracts it.

Looking at microsoft/WSL#4071 and javascript/wsl seems that we can only find it from version information

If its not the correct way then please give me some pointers in the correct direction.

@DilumAluthge
Copy link
Member

Should there be a fallback in case an error occurs while trying to open and/or read from /proc/version?

@mgautam98
Copy link
Contributor Author

mgautam98 commented Jun 25, 2020

Should there be a fallback in case an error occurs while trying to open and/or read from /proc/version?

@DilumAluthge Yes I should wrap it around try-catch

@fredrikekre fredrikekre added needs compat annotation Add !!! compat "Julia x.y" to the docstring needs news A NEWS entry is required for this change needs tests Unit tests are required for this change labels Jun 25, 2020
@StefanKarpinski
Copy link
Member

StefanKarpinski commented Jun 25, 2020

How about this:

detectwsl(os::Symbol) =
    islinux(os) &&
    isfile("/proc/version") &&
    contains(read("/proc/version", String), r"Microsoft"i)

@StefanKarpinski
Copy link
Member

I guess the try-catch might still be good but the main thing is just checking that the file exists.

@StefanKarpinski
Copy link
Member

And actually, if the file exists but we can't read it, what value should be returned? An error might be best. That's a pretty weird situation and if you can't open and read the file, how do you know if you're on WSL or not?

@DilumAluthge
Copy link
Member

Yeah. Is there ever a non-pathological situation in which /proc/version exists but is not world-readable?

@StefanKarpinski
Copy link
Member

I did just try changing the permissions on a Linux system and you can if you have root access change them, so it's possible that someone could make this file not readable to some users. But in that case what's the right answer here? One should generally assume not WSL as the norm, but if you can't read that file we don't know.

@DilumAluthge
Copy link
Member

Does Sys.detectwsl() have to definitely return a Bool? Can it instead return Union{Bool, Missing}? Then, if e.g. we can't read /proc/version, we return missing?

@DilumAluthge
Copy link
Member

(Could replace Missing with Nothing, if you want.)

@StefanKarpinski
Copy link
Member

I feel like that's just deferring the error and making it harder to debug. The inevitable usage of this is to test it with a conditional so that's converting a clear "we cannot tell if this system is running WSL or not" error into an obscure " TypeError: non-boolean (Missing) used in boolean context" error.

@DilumAluthge
Copy link
Member

Hmm. Maybe then throw an error if we can't read the file? At least it will then be clear what the source of the error is.

@StefanKarpinski
Copy link
Member

Yes, that's why my version just checks if the file exists and then reads it. If it can't be read it will produce a fairly clear error and TBH I anticipate this never happening because it would be a pretty odd situation (either the kernel is badly broken and can't produce the contents of /proc/version or someone has changed the permissions on that file to make it unreadable, which, why?).

@DilumAluthge
Copy link
Member

Makes sense to me!

base/sysinfo.jl Outdated Show resolved Hide resolved
@imciner2
Copy link
Contributor

So, the problem is the string microsoft can appear in non-WSL Linux kernels from Microsoft: microsoft/WSL#423 (comment), since apparently some of their cloud images will also contain that string. Instead, the way that seems to be recommended moving forward is to check for the string WSL inside /proc/sys/kernel/osrelease. This seems to be the current "official" way of detecting it for WSL2, since that change comes direct from the WSL repo and devs.

@mgautam98
Copy link
Contributor Author

mgautam98 commented Jun 25, 2020

@imciner2 systemd says its official way of detecting WSL https://github.com/systemd/systemd/pull/15325/files#diff-0e08ed2dbc97131dbf8deb9eb7af4163R465-R471 but for me (Inside WSL2) not havingWSL string

$ cat /proc/sys/kernel/osrelease
4.19.104-microsoft-standard

$ cat /proc/version
Linux version 4.19.104-microsoft-standard (oe-user@oe-host) (gcc version 8.2.0 (GCC)) #1 SMP Wed Feb 19 06:37:35 UTC 2020

/proc/sys/kernel/osrelease is returning a substring of /proc/version

I think both should work

@imciner2
Copy link
Contributor

but for me (Inside WSL2) not havingWSL string

I guess that change hasn't trickled down yet. It looks like it was fairly recently done in the upstream WSL code, so it might not have made it into updaes yet. I would suggest adding a comment in the code referencing the ambiguity we get with the microsoft string (that it could be both a cloud server or WSL), and reference that WSL issue. That way if someone reports incorrect detection of something, this is documented and doesn't have to be researched everytime.

test/osutils.jl Outdated
@@ -28,6 +28,9 @@
else
@test Sys.windows_version() >= v"1.0.0-"
end

@test !Sys.detectwsl(:Windows)
@test !Sys.detectwsl(:Linux)
Copy link
Member

Choose a reason for hiding this comment

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

Won't this test fail if you run it under WSL?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know how we are going to test it. Yes these tests will fail inside WSL how are we going to test it?

Copy link
Member

Choose a reason for hiding this comment

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

I would test that it's not true when the argument is not :Linux since that's always correct and then add this test for the zero-argument version:

if !Sys.islinux()
    @test !Sys.detectwsl()
end

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should the Sys.detectwsl(:Windows) test be removed?

@fonsp
Copy link
Member

fonsp commented Aug 16, 2020

We could call it Sys.iswsl() instead of Sys.detectwsl()? Then it fits in the list of OS checks, and "detect WSL" sounds like an action, not a question - the name does not suggest that it returns a Bool.

@imciner2
Copy link
Contributor

We could call it Sys.iswsl() instead of Sys.detectwsl()? Then it fits in the list of OS checks, and "detect WSL" sounds like an action, not a question - the name does not suggest that it returns a Bool.

See the discussion that occurred in the linked issue about the naming (#36354). The detect prefix was chosen because this is a runtime detection of WSL, not a compile-time constant like all the other sys.is* functions.

@fonsp
Copy link
Member

fonsp commented Aug 16, 2020

Ah sorry, I didn't see it - I can see why Sys.is* should be reserved.

But I still feel like detectwsl is semantically off - it sounds like it has return type "Union{WSL, Missing}", not Bool. To me, it is comparable to findnext, getfield and getkey, which all return the found index/object, not whether or not it was found.

Some final suggestions:

  • Sys.runtimeiswsl() (and Sys.runtimeiscontainer(), etc)

or we return a struct:

struct RuntimeProperties
	WSL::Bool
	container::Bool
	etc
end

Sys.runtimeproperties()::RuntimeProperties = ...

Just wanted to give some alternatives, but feel free to ignore :)

@StefanKarpinski
Copy link
Member

Another possible name would be Sys.checkwsl() if you think that would be clearer?

@fonsp
Copy link
Member

fonsp commented Aug 17, 2020

Yeah somehow that sounds a lot better... I like it!

@fonsp
Copy link
Member

fonsp commented Aug 17, 2020

Should it take os::Symbol as an argument? Or is it there for convenience?

@StefanKarpinski
Copy link
Member

It's a little weird to pass the symbol in this case since you can't really simulate being on another system with this.

"""
Sys.detectwsl([os])

Runtime Predicate for testing if Julia is running inside WSL.
Copy link
Member

Choose a reason for hiding this comment

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

How about this? (The 1.11 would have to be changed if this were to be backported to 1.10)

Suggested change
Runtime Predicate for testing if Julia is running inside WSL.
Runtime predicate for testing if Julia is running inside
Windows Subsystem for Linux (WSL).
!!! note
Unlike `Sys.iswindows`, `Sys.islinux` etc., this is a runtime tests, and thus
cannot meaningfully be used in `@static if ` constructs.
!!! compat "Julia 1.11"
This function requires at least Julia 1.11.

@fingolfin
Copy link
Member

This has sat here for a long time unfortunately. So I don't know if @mgautam98 still has any interest in this? If so I'd be happy to help get it over the line. If not perhaps someone would like to "adopt" it. Otherwise we'll have to close it :-(

@fingolfin fingolfin added the waiting for author Anybody home? label Oct 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs compat annotation Add !!! compat "Julia x.y" to the docstring needs news A NEWS entry is required for this change needs tests Unit tests are required for this change waiting for author Anybody home?
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants