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

Remove implicit dependency on wslvar binary (#198) #204

Closed
wants to merge 1 commit into from

Conversation

tim-stasse
Copy link
Contributor

@tim-stasse tim-stasse commented Oct 23, 2020

Fixes #198
Fixes #200

@tim-stasse
Copy link
Contributor Author

I believe this would make 200 redundant (if accepted).

const wslAutoMountRoot = async () => {
let rootDirectory = '/mnt/';
try {
const {stdout} = await pExecFile('awk', [
Copy link
Owner

Choose a reason for hiding this comment

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

Use JS for the parsing instead of awk.

const paths = await pReaddir(rootDirectory);

/* eslint-disable no-await-in-loop */
for (const path of paths) {
Copy link
Owner

Choose a reason for hiding this comment

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

This can be done concurrently using Promise.all.

@sindresorhus
Copy link
Owner

Can you describe your changes and why they're required?

Copy link
Contributor

@jonaskuske jonaskuske left a comment

Choose a reason for hiding this comment

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

Since this already adds a detection for wslAutoMountRoot, you could use that as well for the check on line 125 instead of hardcoding /mnt/ – that'd make my PR (#210) unnecessary :)

@sindresorhus
Copy link
Owner

@tim-stasse Friendly bump :)

Base automatically changed from master to main January 24, 2021 06:10
@anaisbetts
Copy link
Contributor

anaisbetts commented Jan 29, 2021

@sindresorhus I think I can explain - wslvar is not a built-in component, it's optional software that many people don't have. While Powershell is guaranteed to exist, wslvar is very much not. fwiw, this bug breaks popular software such as Storybook on WSL because open fails

An easy way to replace wslvar is to just do something similar to https://github.com/anaisbetts/ssh-agent-relay/blob/master/bin/ssh-agent-relay#L15, use cmd.exe to echo the Windows environment variable

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.

Error launching chrome from WSL
4 participants