Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

feat(wsl): detect wsl #36

Merged
merged 4 commits into from
Feb 17, 2025
Merged

feat(wsl): detect wsl #36

merged 4 commits into from
Feb 17, 2025

Conversation

pankgeorg
Copy link
Member

@pankgeorg pankgeorg commented Jan 29, 2025

Allows opening links in Windows' browser from wsl

mortenpi
mortenpi previously approved these changes Feb 10, 2025
Copy link
Member

@mortenpi mortenpi left a comment

Choose a reason for hiding this comment

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

This LGTM. I tested this on my Windows machine, and I can both repro the original problem and confirm that this seems to fix it.

src/helpers.jl Outdated
Comment on lines 3 to 7
function detectwsl()
Sys.islinux() &&
isfile("/proc/sys/kernel/osrelease") &&
occursin(r"Microsoft|WSL"i, read("/proc/sys/kernel/osrelease", String))
end
Copy link
Member

@mortenpi mortenpi Feb 10, 2025

Choose a reason for hiding this comment

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

This does a bunch of filesystem operations. In general, it would probably better to memoize this into a global I think. But in this case, we don't really call detectwsl that many times, so this seems fine to me as-is too.

@mortenpi mortenpi requested a review from pfitzseb February 10, 2025 06:23
@mortenpi
Copy link
Member

mortenpi commented Feb 10, 2025

Note: CI failure is persistent, but unrelated -- it looks like some weird linking issue with Julia 1.4 and MacOS. We should maybe just stop testing for some older Julia versions?

src/helpers.jl Outdated
Comment on lines 3 to 7
function detectwsl()
Sys.islinux() &&
isfile("/proc/sys/kernel/osrelease") &&
occursin(r"Microsoft|WSL"i, read("/proc/sys/kernel/osrelease", String))
end
Copy link
Member

Choose a reason for hiding this comment

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

I'd be more comfortable with some error handling. Thoughts on this?

Suggested change
function detectwsl()
Sys.islinux() &&
isfile("/proc/sys/kernel/osrelease") &&
occursin(r"Microsoft|WSL"i, read("/proc/sys/kernel/osrelease", String))
end
function detectwsl()
Sys.islinux() || return false
try
return occursin(r"Microsoft|WSL"i, read("/proc/sys/kernel/osrelease", String))
catch err
@debug "Could not read /proc/sys/kernel/osrelease." exception=(err, catch_backtrace())
end
return false
end

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure. I also just found out that there is a new way to do this: https://github.com/JuliaLang/julia/pull/57069/files

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, that's neat.

@static if isdefined(Sys, :detectwsl)
    const detectwsl = Sys.detectwsl
else
    # attribution
    function detectwsl()
        # We use the same approach as canonical/snapd do to detect WSL
        islinux() && (
            isfile("/proc/sys/fs/binfmt_misc/WSLInterop")
            || isdir("/run/WSL")
        )
    end
end

then?

Copy link
Member Author

Choose a reason for hiding this comment

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

seems great!

Copy link
Member

Choose a reason for hiding this comment

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

I took the liberty up updating the PR with this.

@mortenpi mortenpi dismissed their stale review February 17, 2025 02:00

N/A anymore

@mortenpi mortenpi requested a review from pfitzseb February 17, 2025 02:00
@mortenpi mortenpi enabled auto-merge (squash) February 17, 2025 02:01
@mortenpi mortenpi merged commit 04421fe into master Feb 17, 2025
14 checks passed
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants