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

Fix resolveBin for windows #8

Closed
kentcdodds opened this issue Oct 12, 2017 · 8 comments · Fixed by #88
Closed

Fix resolveBin for windows #8

kentcdodds opened this issue Oct 12, 2017 · 8 comments · Fixed by #88

Comments

@kentcdodds
Copy link
Owner

resolveBin:

kcd-scripts/src/utils.js

Lines 21 to 44 in bd3d642

function resolveBin(modName, {executable = modName, cwd = process.cwd()} = {}) {
let pathFromWhich
try {
pathFromWhich = fs.realpathSync(which.sync(executable))
} catch (_error) {
// ignore _error
}
try {
const modPkgPath = require.resolve(`${modName}/package.json`)
const modPkgDir = path.dirname(modPkgPath)
const {bin} = require(modPkgPath)
const binPath = typeof bin === 'string' ? bin : bin[executable]
const fullPathToBin = path.join(modPkgDir, binPath)
if (fullPathToBin === pathFromWhich) {
return executable
}
return fullPathToBin.replace(cwd, '.')
} catch (error) {
if (pathFromWhich) {
return executable
}
throw error
}
}

It's responsible for taking a module name (like eslint) and getting a path to the executable binary. If the path to the executable is the same as the one that's in the $PATH environment variable (using which), then we can return the raw binary name itself and execute a script with that, otherwise we'll return the full path so it's executed with the full path (because it's not available in $PATH presumably.

This is useful because if the user sees the script we run (which they do in some situations) then the script will be shorter.

It works on linux systems right now, but apparently the result of which.sync is to a file ending in .CMD and isn't a symlink (so the realpathSync doesn't resolve it to the binary path).

So we need to find a package that will do this for us (I already tried resolve-bin and it didn't work the way I want it to) or fix this function up to make it work for windows as well as linux.

Anyone wanna help out?

@rossipedia
Copy link

Ok I'm not sure I understand what the problem is. The executable for Windows is the *.cmd file, which is a thin wrapper around invoking node.exe with pkg.bin.

For example, the contents of node_modules\.bin\webpack.cmd is:

@IF EXIST "%~dp0\node.exe" (
  "%~dp0\node.exe"  "%~dp0\..\webpack\bin\webpack.js" %*
) ELSE (
  @SETLOCAL
  @SET PATHEXT=%PATHEXT:;.JS;=;%
  node  "%~dp0\..\webpack\bin\webpack.js" %*
)

and likewise, the contents of which.cmd is:

@IF EXIST "%~dp0\node.exe" (
  "%~dp0\node.exe"  "%~dp0\..\which\bin\which" %*
) ELSE (
  @SETLOCAL
  @SET PATHEXT=%PATHEXT:;.JS;=;%
  node  "%~dp0\..\which\bin\which" %*
)

As you can see, they follow the same general format, the only thing differing is the path to the *.js file that node will execute.

This is required because in Windows, executable files are determined by file extension rather than a permissions bit as they are in *nix land.

The *.cmd file can be treated as the executable, and can be started via child_process.exec or child_process.spawn and it should just do the right thing.

@rossipedia
Copy link

Ok, I see what the issue is (I cloned the repo and ran the test suite). You might have to rethink the tests here, since the way Windows handles command lines / dir separators / executables is so different from *nix. Gotta run to the store, but I might take a crack at it later tonight.

@kentcdodds
Copy link
Owner Author

Thanks for looking into it! If you have another suggestion let me know! I'm thinking that with the serializers I add we can normalize the paths so it's not an issue for tests, but I definitely want it to work the same as far as the implementation goes. I'd really love it to not have the full path to the executable if the executable exists in the PATH by its name already.

@alexandernanberg
Copy link
Contributor

I might be missing something, but why not use npx to run the scripts instead? From what I've seen that should remove the need for the resolveBin util altogether.

@kentcdodds
Copy link
Owner Author

Hmmm... Interesting thought. That might work! Would you like to give it a try?

@alexandernanberg
Copy link
Contributor

Sure, I'll give it a go!

@kentcdodds
Copy link
Owner Author

🎉 This issue has been resolved in version 1.3.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@adamdharrington
Copy link
Contributor

nice one @kentcdodds !

layershifter pushed a commit to layershifter/kcd-scripts that referenced this issue Mar 29, 2021
…entcdodds#8)

* Use originalValue instead of object[originalKey]

* add noflip switch

* add test for noflip switch

* add docs for noflip switch

* add myself to the list of contributors 😬
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants