eslint-interactive icon indicating copy to clipboard operation
eslint-interactive copied to clipboard

command failed in Windows system

Open lewis-geek opened this issue 2 years ago • 1 comments

I can not run any command successfully, for example, I was running eslint-interactive --help, and an error occurred, the error message is below.

image

I have tried other commands like eslint-interactive ./src, same error happened.

I was running the commands in Windows 10 system.

lewis-geek avatar Jun 24 '22 02:06 lewis-geek

Thanks for the bug report! Sorry, it is difficult to reproduce or fix the problem because I do not have a windows machine.

But you are welcome to send pull-request!

mizdra avatar Jul 05 '22 16:07 mizdra

Same on macOS. The line is:

exec -S  node --enable-source-maps --unhandled-rejections=strict --experimental-import-meta-resolve "$basedir/../eslint-interactive/bin/eslint-interactive.js" "$@"
$ exec -S
exec: -S: unknown option

Does this only work on Linux?

KnifeFed avatar Oct 03 '22 18:10 KnifeFed

@KnifeFed Thanks for reporting the problem!

I think the problem stems from another cause, not windows. Could you open another issue?

Also, the command example you are providing appears to be incorrect. The -S is an option for /usr/bin/env command, not exec command. Could you switch to the env command and try again?

mizdra avatar Oct 10 '22 12:10 mizdra

@mizdra The issue in @lewis-geek's screenshot is '-S' is not recognized as an internal or external command when /node_modules/.bin/eslint-interactive is run, which is the same issue I'm having. I'm just running pnpm exec eslint-interactive --help as per the docs and ./node_modules/.bin/eslint-interactive is in turn running:

exec -S  node --enable-source-maps --unhandled-rejections=strict --experimental-import-meta-resolve "$basedir/../eslint-interactive/bin/eslint-interactive.js" "$@"

So the issue seems to be in how that file is generated.

KnifeFed avatar Oct 10 '22 14:10 KnifeFed

The issue comes from this line I think: https://github.com/mizdra/eslint-interactive/blob/main/bin/eslint-interactive.js#L1

Potentially this should already fix it:

#!/usr/bin/env node --enable-source-maps --unhandled-rejections=strict --experimental-import-meta-resolve

but I'm not sure why an even simpler

#!/usr/bin/env node

shouldn't do

dbartholomae avatar Dec 09 '22 20:12 dbartholomae

@dbartholomae Thanks for the proposal! However, I don't think we should remove the options. All those options have a role.

  • --enable-source-maps (for node)
    • When eslint-interactive encounters an internal error and outputs a stack trace, that option is used to show the location on .ts instead of .js.
  • --unhandled-rejections=strict (for node)
    • In Node.js v14.x and below, when unhandled rejections occur, the program does not exit and continues to live like a zombie.
    • We want to terminate the program immediately, as continuing to run an abnormal program is likely to cause serious problems.
    • This option causes the program to exit immediately when unhandled rejections occur.
    • FYI: In Node.js v15 and above, the program will exit by default when unhandled rejections occur. However, eslint-interactive also supports Node.js 14.x.
  • --experimental-import-meta-resolve (for node)
    • Required to use import.meta.resolve.
  • -S (for /usr/bin/env)
    • I don't remember exactly why I added this... #95 says it is a workaround for Ubuntu.

mizdra avatar Dec 11 '22 05:12 mizdra

Thanks! All I can say is that this change make the tool work on Windows (with Node 16), and that these are the settings used by eslint itself. I can't test on Ubuntu to know whether this somehow makes the tool stop working on Ubuntu, but if it doesn't, I would at least get rid of the -S. If you do not plan to support Windows, that's also fine, just let me know - in that case I would most likely open a fork that supports windows and maintain it seperately.

dbartholomae avatar Dec 11 '22 10:12 dbartholomae

I think Windows support is just as important as Ubuntu and Mac. Therefore, I welcome the pull request!

However, I cannot accept the pull request if it does not pass the test. In order to accept the pull request, could you please try the following on your windows machine?

  1. run pnpm run test to check if it passes.
  2. run pnpm run build && pnpm run postbuild:test and check if it passes.

import.meta.resolve is called when Display details of lint results is executed. So without the --experimental-import-meta-resolve option, the following test would fail.

https://github.com/mizdra/eslint-interactive/blob/500d424d7762bce7801c5bd1f0e00e21c5f310f3/src/core.test.ts#L137-L140

Could you check to see if the test actually fails, and if so, could you fix it? I do not have a windows machine and cannot test it.

mizdra avatar Dec 11 '22 11:12 mizdra

Thanks!

I've run the tests (with some small changes, as diff, cp, and rf don't work on Windows, and the snapshots assume / as path delimiter while Windows uses \), and they pass (except for the ones using diff, I didn't find a fast workaround to replace the diff command with a windows command).

But this unfortunately doesn't confirm whether the change works, as the changed file isn't actually tested. The same for building: The only file that changes isn't built, so builds still give the exact same output as before the PR.

What I did to confirm the change was to install the changed version and run it manually on Windows. This worked. Based on your feedback, I've also rechecked with Node 14.13.1, which also works. I've also checked via a Codespace on Ubuntu (Ubuntu 22.04.1 LTS) with Node 18.12.1 and 14.13.1, and it works. These are manual tests, so I might have missed some corner cases, but for the workflow of linting, choosing a rule, auto-fixing it, and exiting, I didn't notice any problems.

In the end, there are also a couple of separate decisions to be made: All options, except for -S, can also be kept and the script would still run under Windows. For the first two, this is mainly a question of preference, so I would adapt the PR to add them back if you want to keep them. For the third, based on the manual testing it seems that the option isn't actually needed - but I might be missing an edge case, and, again, it would be possible to just leave it in. The problematic one is -S, as this is the one that stops the script from working on Windows. If I missed something when manually testing on Ubuntu, and the option is indeed needed to run there, then this change should not be merged and further investigation is needed.

PS: I could also offer to set up a GitHub workflow that runs the tests on each PR to make it easier for you to confirm that the tests are green.

dbartholomae avatar Dec 11 '22 12:12 dbartholomae

I recently got a windows machine and was working on this problem.

And I just released eslint-interactive v10.4.0 which fixes this problem! There are a few known issues (https://github.com/mizdra/eslint-interactive/issues/252), but most of the functionality should work. Please check it out.

Thanks for contributing to this issue! @dbartholomae @lewis-geek @KnifeFed

mizdra avatar Jan 09 '23 15:01 mizdra

Hi @mizdra

I tested it in the windows environment, it works now,

Thank you for fixing the issue!

lewis-geek avatar Jan 11 '23 07:01 lewis-geek