-
Notifications
You must be signed in to change notification settings - Fork 877
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
Use a script to proxy PNPM executable on Windows #1116
Conversation
for (String script : Arrays.asList("pnpm", "pnpm.cmd")) { | ||
File scriptFile = new File(pnpmDirectory, "bin" + File.separator + script); | ||
if (scriptFile.exists()) { | ||
File copy = new File(installDirectory, script); | ||
if (!copy.exists()) { | ||
try | ||
{ | ||
FileUtils.copyFile(scriptFile, copy); | ||
} | ||
catch (IOException e) | ||
{ | ||
throw new InstallationException("Could not copy pnpm", e); | ||
} | ||
copy.setExecutable(true); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have removed this old code as it seems to have been copied from the NpmInstaller
, but never actually functioned. I've manually checked every release of PNPM, and none of them include a pnpm
or pnpm.cmd
file in their bin
directory.
String scriptContents = new StringBuilder() | ||
.append(":: Created by frontend-maven-plugin, please don't edit manually.\r\n") | ||
.append("@ECHO OFF\r\n") | ||
.append("\r\n") | ||
.append("SETLOCAL\r\n") | ||
.append("\r\n") | ||
.append(String.format("SET \"NODE_EXE=%%~dp0\\%s\"\r\n", relativeNodePath)) | ||
.append(String.format("SET \"PNPM_CLI_JS=%%~dp0\\%s\"\r\n", relativePnpmPath)) | ||
.append("\r\n") | ||
.append("\"%NODE_EXE%\" \"%PNPM_CLI_JS%\" %*") | ||
.toString(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This script is partially taken from the NPM equivalent, but pruned down to the bare essentials needed for this use-case.
12ee55a
to
4e098a1
Compare
4e098a1
to
ceb0068
Compare
Looks like the tests on Windows are flaking out on something unrelated to the changes in this PR? Perhaps a re-run will pass them. |
Looks like it works. Thanks! |
Thanks for getting to this so quick @eirslett, much appreciated. |
Could you please help update the changelog as well? I think this will be released as a patch version. |
Sure, opened a PR under #1117 |
Since Windows does not support symbolic links (or at least not consistently), the choice was made not to support the PNPM executable under Windows. This PR adds support for running the PNPM executable by adding a script under the same name in the installation directory, and proxy any commands sent to it to the PNPM executable.
This also allow the removal of some of the testing profiles that were previously added as the tests would not pass under Windows. And also removes the legacy linking code that was copied from the NPM version that never worked.
Closes #1115