search

eirslett / frontend-maven-plugin

"Maven-node-grunt-gulp-npm-node-plugin to end all maven-node-grunt-gulp-npm-plugins." A Maven plugin that downloads/installs Node and NPM locally, runs NPM install, Grunt, Gulp and/or Karma.
Apache License 2.0
4.21k stars 867 forks source link

Fix #967 : Add pnpm executable to PATH #1045

Closed JulesAaelio closed 1 year ago

JulesAaelio commented 1 year ago

Summary

Fix the issue described in #967

the PNPMInstaller tries to copy pnpm & pnpm.cmd (copyPnpmScripts), but pnpm has no such predefined files, thus it does not find anything to copy.

The solution I propose is to create a symlink to node_modules/pnpm/bin/pnpm.cjs when no pnpm or pnpm.cmd is available.

Tests and Documentation

I tested using this dummy project : https://github.com/JulesAaelio/test-frontend-maven-plugin

eirslett commented 1 year ago

How will this work on Windows (which doesn't have symlinks, I think)?

GuillaumeC commented 1 year ago

pnpm work with symlinks to a global store. So it should be work on Windows too.

JulesAaelio commented 1 year ago

Hi @eirslett , You have a valid point here, although symlinks are supposed to work on Windows 10 it is still unclear under what conditions.

According to their documentation, pnpm does'nt use symlinks neither, but junctions, that we cannot use here because we're trying to create a link to a file and not to a directory. https://pnpm.io/faq#does-it-work-on-windows

The others options to make this works on windows are : As stated in the issue :

to execute cmd-shim if the files are not found. to install the .tgz files with npm instead of just extracting them, which I assume would add the executables automatically. But of course that would require the npm to be installed in advance.

Or : to use the freshly installed pnpm to "install" itself so it creates the executables.

=> As all of these are non-trivial, I propose to simply add a check before creating the symlink to ensure that we're running on a non-windows platform. I updated my code accordingly .

eirslett commented 1 year ago

:+1:

However, this project has an integration test that tries to run pnpm. As far as I can see, that integration test is now passing. Is it possible to create a reproduction of this bug/issue (a failing integration test), to show that the problem has indeed been fixed with this patch?

JulesAaelio commented 1 year ago

Sure, I can have a look. Should I include the new test in this merge request or in a new one ? EDIT: Added in #1053

eirslett commented 1 year ago

@JulesAaelio I merged your PR with the failing integration test. As you can see here: https://github.com/eirslett/frontend-maven-plugin/actions/runs/3169669688/jobs/5161739948 it looks like the integration test is still failing on Ubuntu? Can you have a look?

If you rebase your pull request branch, the integration test should be included in there, so it's easier to check whether the test passes or fails.

JulesAaelio commented 1 year ago

Hi @eirslett Looks like the integration tests on ubuntu were failing due to a version mismatch between pnpm and node (Seems like pnpm compatibility table is not up to date).

=> I bumped the versions and the tests are now behaving as expected : Failing on windows and passing on ubuntu The macos job was cancelled because the windows job failed

eirslett commented 1 year ago

:+1: So how do we make the test pass on Windows then?

JulesAaelio commented 1 year ago

As I said on my previous comment, making it work on windows is a bit more complicated and I don't feel like digging into it. So I used a profile to ensure the test doesn't run. It is not really a fix but the tests are passing. Let me know what you think :)

eirslett commented 1 year ago

Ok, let's merge it anyways! And hopefully somebody on Windows will fix it if they need it.