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

Ensure node is accessible before running npm #1486

Closed
wants to merge 3 commits into from

Conversation

jnels124
Copy link

@jnels124 jnels124 commented Jan 13, 2023

Fixes #1185
Please DO NOT FORCE PUSH. Don't worry about messy history, it's easier to do code review if we can tell what happened after the review, and force pushing breaks that.

Please make sure that your PR allows edits from maintainers. Sometimes its faster for us to just fix something than it is to describe how to fix it.

Allow edits from maintainers

After creating the PR, please add a commit that adds a bullet-point under the [Unreleased] section of CHANGES.md, plugin-gradle/CHANGES.md, and plugin-maven/CHANGES.md which includes:

  • a summary of the change
  • either
    • a link to the issue you are resolving (for small changes)
    • a link to the PR you just created (for big changes likely to have discussion)

If your change only affects a build plugin, and not the lib, then you only need to update the plugin-foo/CHANGES.md for that plugin.

If your change affects lib in an end-user-visible way (fixing a bug, updating a version) then you need to update CHANGES.md for both the lib and all build plugins. Users of a build plugin shouldn't have to refer to lib to see changes that affect them.

This makes it easier for the maintainers to quickly release your changes :)

@jnels124
Copy link
Author

jnels124 commented Jan 13, 2023

This PR addresses the issue specified in #1185. When setting the npm executable directly and node is not accessible on the $PATH, the npm executable is not able to run. This PR checks to ensure that we are using the node version in the same directory as npm. Admittedly, this change could be implemented to be more flexible and possibly have different path resolvers but not sure that functionality is needed so went with the smallest scope of change possible. Could also just update the process builder to modify the PATH env var to include the parent folder of the npm executable

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

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.

@simschla
Copy link
Contributor

Changes in this PR should be obsolete by merging #1500

Better integration of gradle-node-plugin and spotless is discussed in #1499 - feel free to follow there.

Thanks again for the PR, I'm closing without merging.

@simschla simschla closed this Jan 19, 2023
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.

Prettier doesn't work if node is not on the PATH
2 participants