Skip to content

Ensure node is accessible before running npm #1486

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.

Sign up for GitHub

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

Closed
wants to merge 3 commits into from
Closed

Ensure node is accessible before running npm #1486

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
615badd
Ensure node is accessible before running npm
jnels124 Jan 13, 2023
0fb922f
Add change log notes
jnels124 Jan 13, 2023
e79843a
Merge remote-tracking branch 'upstream/main' into prettier-fix
jnels124 Jan 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions CHANGES.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@ We adhere to the [keepachangelog](https://keepachangelog.com/en/1.0.0/) format (
* Better suggested messages when user's default is set by JVM limitation. ([#995](https://github.com/diffplug/spotless/pull/995))
### Fixed
* Support `ktlint` 0.48+ new rule disabling syntax ([#1456](https://github.com/diffplug/spotless/pull/1456)) fixes ([#1444](https://github.com/diffplug/spotless/issues/1444))
### Fixed
* Support for executing prettier without node being present on $PATH [#1185](https://github.com/diffplug/spotless/issues/1185)
### Changes
* Bump default version for `prettier` from `2.0.5` to `2.8.1` ([#1453](https://github.com/diffplug/spotless/pull/1453))
* Bump the dev version of Gradle from `7.5.1` to `7.6` ([#1409](https://github.com/diffplug/spotless/pull/1409))
Expand All @@ -31,6 +33,7 @@ We adhere to the [keepachangelog](https://keepachangelog.com/en/1.0.0/) format (
* Bump default `ktlint` version to latest `0.47.1` -> `0.48.1` ([#1456](https://github.com/diffplug/spotless/pull/1456))
* Switch our publishing infrastructure from CircleCI to GitHub Actions ([#1462](https://github.com/diffplug/spotless/pull/1462)).
* Help wanted for moving our tests too ([#1472](https://github.com/diffplug/spotless/issues/1472))
* Execute npm using node executable in same directory


## [2.31.1] - 2023-01-02
Expand Down
6 changes: 6 additions & 0 deletions lib/src/main/java/com/diffplug/spotless/npm/NpmProcess.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,8 +73,14 @@ private Process npm(String... args) {

private List<String> processCommand(String... args) {
List<String> command = new ArrayList<>(args.length + 1);
File nodeExecutable = this.npmExecutable.getParentFile().toPath().resolve("node").toFile();
if (!(nodeExecutable.exists() && nodeExecutable.canExecute())) {
throw new IllegalStateException("node must exist in the same directory as npm and be executable. Using npm @ " + this.npmExecutable.getAbsolutePath());
}
command.add(nodeExecutable.getAbsolutePath());
Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for this PR. I'm not sure we can keep the code like this. Two reasons:

  1. In Prettier doesn't work if node is not on the PATH #1185 you describe, that you want to use a node installation dynamically retrieved via node-gradle-plugin. With that plugin specifically it is possible, to download npm and node to different locations:
node {
    download = true
    version = '18.13.0'
    npmVersion = '8.19.2'
    workDir = file("${buildDir}/nodejs")
    npmWorkDir = file("${buildDir}/npm")
}

This change would prevent such an installation from ever working with spotless prettier.
2. I think on windows, the node binary is called node.cmd, so this test would probably fail there.

I would suggest to change your PR to detect when the npm process fails due to missing node binary and give the user a very specific error message for this.

Regarding being able to better integrate node-gradle-plugin: I'm currently looking into allowing the user to specify PATH and/or the location of the node binary (triggered by #1381).

Copy link
Author

@jnels124 jnels124 Jan 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Can update the error message but the problem still remains. NPM can't run without node on the $PATH and that is not desirable. Looks like the work described in #1381 completely solves the issue with executing npm without node being on the System $PATH but there are other issues that need to be solved in order to make the node gradle plugin work for Prettier. I added some comments on #1381 in reference to the additional issues that need to be resolved.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can update the error message but the problem still remains.
Maybe you misinterpreted what I meant.

I thought of something along the way of

try {
    // npm install is triggered
} catch (Exception e) {
   if (e.getMessage().contains("failed with exit code: 127")) {
      throw new IllegalStateException("Node could not be found on the path, npm is unable to execute.");
  }
}

But with the changes I'm currently doing for #1381, that change would become obsolete. With that said, are you OK if we close this PR?

For the second part of your reply:
I agree with you: we have an issue about config-phase / execution phase when interoperating spotless and gradle-node-plugin. I suggest we raise a new issue for that topic and move discussion there.

command.add(this.npmExecutable.getAbsolutePath());
command.addAll(Arrays.asList(args));

return command;
}

Expand Down
5 changes: 3 additions & 2 deletions plugin-gradle/CHANGES.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,15 +15,16 @@ We adhere to the [keepachangelog](https://keepachangelog.com/en/1.0.0/) format (
### Changes
* Bump default `ktlint` version to latest `0.47.1` -> `0.48.1` ([#1456](https://github.com/diffplug/spotless/pull/1456))
* Bump default version for `prettier` from `2.0.5` to `2.8.1` ([#1453](https://github.com/diffplug/spotless/pull/1453))

### Fixed
* Support for executing prettier without node being present on $PATH [#1185](https://github.com/diffplug/spotless/issues/1185)
## [6.12.1] - 2023-01-02
### Fixed
* Improve memory usage when using git ratchet ([#1426](https://github.com/diffplug/spotless/pull/1426))
* Support `ktlint` 0.48+ ([#1432](https://github.com/diffplug/spotless/pull/1432)) fixes ([#1430](https://github.com/diffplug/spotless/issues/1430))
### Changes
* Bump default `ktlint` version to latest `0.47.1` -> `0.48.0` ([#1432](https://github.com/diffplug/spotless/pull/1432))
* Bump default `ktfmt` version to latest `0.41` -> `0.42` ([#1421](https://github.com/diffplug/spotless/pull/1421))

* Execute npm using node executable in same directory
## [6.12.0] - 2022-11-24
### Added
* `importOrder` now support groups of imports without blank lines ([#1401](https://github.com/diffplug/spotless/pull/1401))
Expand Down
5 changes: 3 additions & 2 deletions plugin-maven/CHANGES.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,8 @@ We adhere to the [keepachangelog](https://keepachangelog.com/en/1.0.0/) format (
* Bump default `ktlint` version to latest `0.47.1` -> `0.48.1` ([#1456](https://github.com/diffplug/spotless/pull/1456))
* Reduce spurious invalidations of the up-to-date index file ([#1461](https://github.com/diffplug/spotless/pull/1461))
* Bump default version for `prettier` from `2.0.5` to `2.8.1` ([#1453](https://github.com/diffplug/spotless/pull/1453))

### Fixed
* Support for executing prettier without node being present on $PATH [#1185](https://github.com/diffplug/spotless/issues/1185)
## [2.29.0] - 2023-01-02
### Added
* Added support for M2E's incremental compilation ([#1414](https://github.com/diffplug/spotless/pull/1414) fixes [#1413](https://github.com/diffplug/spotless/issues/1413))
Expand All @@ -27,7 +28,7 @@ We adhere to the [keepachangelog](https://keepachangelog.com/en/1.0.0/) format (
### Changes
* Bump default `ktlint` version to latest `0.47.1` -> `0.48.0` ([#1432](https://github.com/diffplug/spotless/pull/1432))
* Bump default `ktfmt` version to latest `0.41` -> `0.42` ([#1421](https://github.com/diffplug/spotless/pull/1421))

* Execute npm using node executable in same directory
## [2.28.0] - 2022-11-24
### Added
* `importOrder` now support groups of imports without blank lines ([#1401](https://github.com/diffplug/spotless/pull/1401))
Expand Down