Bugfix for issue #667 - devon npm setup ignores a given version #668
Conversation
There was a problem hiding this comment.
@fdcg thanks for this great PR 👍
I did some minimal changes directly as constructive review.
For the download we should follow the new approach as commented in the review below.
Should I take over or would have have the chance/time to rework?
UPDATE: Did you also test my quickfix that has already been released?
| else | ||
| local download_url="https://registry.npmjs.org/npm/-/npm-${software_version}.tgz" | ||
| local software_dir="${NODE_HOME}/node_modules/npm" | ||
| doInstall "${software_dir}" "${download_url}" "npm" "${software_version}" |
There was a problem hiding this comment.
this will not work as you have developed this PR for an older version of devonfw-ide and the function and download process has been improved and refactored. See: #641
So we should to put the URL into https://github.com/devonfw/ide-mirrors/ and change the arguments of doInstall (see other commandlets that have been refactored already).
There was a problem hiding this comment.
As a quickfix, I changed the order of the parameters so it should already work, but we want to keep the download URLs out of our release as explained in #641
There was a problem hiding this comment.
I have already added the according config to the ide-mirrors repo:
https://github.com/devonfw/ide-mirrors/tree/master/npm
There was a problem hiding this comment.
so we could stip out the download_url (line 36) and change to:
doInstall "-" "${software_dir}" "npm" "${software_version}"
| if [ -f "${NODE_HOME}/npm" ] | ||
| then | ||
| rm "${NODE_HOME}/npm" | ||
| fi | ||
| if [ -f "${NODE_HOME}/npm.cmd" ] | ||
| then | ||
| rm "${NODE_HOME}/npm.cmd" | ||
| fi | ||
| if [ -f "${NODE_HOME}/npx" ] | ||
| then | ||
| rm "${NODE_HOME}/npx" | ||
| fi | ||
| if [ -f "${NODE_HOME}/npx.cmd" ] | ||
| then | ||
| rm "${NODE_HOME}/npx.cmd" | ||
| fi | ||
| cp "${software_dir}/bin/npm" "${NODE_HOME}" | ||
| cp "${software_dir}/bin/npm.cmd" "${NODE_HOME}" | ||
| cp "${software_dir}/bin/npx" "${NODE_HOME}" | ||
| cp "${software_dir}/bin/npx.cmd" "${NODE_HOME}" |
There was a problem hiding this comment.
Instead of removing and copying, I would prefer to create symlinks but due to windows limitations this does not seem to be an option. So I think there is no better solution than yours here that should always work (KISS).
There was a problem hiding this comment.
For the record: the current approach could fail on Windows if a npm.cmd is currently running in parallel due to file locking.
hohwille
added
enhancement
|
@fdcg awesome. Thanks for updating this PR and testing on win. |
No description provided.