-
Notifications
You must be signed in to change notification settings - Fork 359
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
[node] Run pnpm setup after install #770
base: main
Are you sure you want to change the base?
Conversation
@microsoft-github-policy-service agree |
src/node/install.sh
Outdated
@@ -232,6 +232,8 @@ else | |||
[ ! -z "$https_proxy" ] && npm set https-proxy="$https_proxy" | |||
[ ! -z "$no_proxy" ] && npm set noproxy="$no_proxy" | |||
npm install -g pnpm | |||
pnpm setup |
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.
adds the pnpm home directory to the PATH by updating the shell configuration file
From https://pnpm.io/cli/setup, and looking at all the test failures, looks like this is not working as expected. See https://github.com/devcontainers/features/actions/runs/7043208217/job/19189866015?pr=770#step:4:679
Curious, can you help me understand the motivation behind this PR? Wondering if this is helping with faster pmnp configurations.
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.
oops, my motivation is that when I open a project in a nodejs devcontainer, from reading https://pnpm.io/npmrc#node-modules-settings I expect the pnpm store-dir to be set in the following order:
If the $PNPM_HOME env variable is set, then $PNPM_HOME/store
If the $XDG_DATA_HOME env variable is set, then $XDG_DATA_HOME/pnpm/store
On Windows: ~/AppData/Local/pnpm/store
On macOS: ~/Library/pnpm/store
On Linux: ~/.local/share/pnpm/store
What actually happens is that since $PNPM_HOME is not set yet (happens during pnpm setup
from my understanding), that the pnpm store-dir is set to the project root
Checkout this issue: pnpm/pnpm#7100. In my opinion, since pnpm is installed I would expect that it would use the first option $PNPM_HOME/store
and I wouldn't need to add .pnpm-store/
to my .gitignore
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.
Thanks for the explanation.
Can you add few tests to validate your changes?
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.
Apologies for the delay in the PR review. Left few comments, thanks!
@@ -232,7 +232,7 @@ else | |||
[ ! -z "$https_proxy" ] && npm set https-proxy="$https_proxy" | |||
[ ! -z "$no_proxy" ] && npm set noproxy="$no_proxy" | |||
npm install -g pnpm | |||
pnpm setup | |||
ENV="$HOME/.bashrc" SHELL="$(which bash)" pnpm setup |
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 am not confident if $HOME
would resolve correctly. Hence, I think you need to compute Home Dir as similar to
features/src/common-utils/main.sh
Lines 418 to 429 in 65fb90b
if [ "${USERNAME}" = "root" ]; then | |
user_home="/root" | |
# Check if user already has a home directory other than /home/${USERNAME} | |
elif [ "/home/${USERNAME}" != $( getent passwd $USERNAME | cut -d: -f6 ) ]; then | |
user_home=$( getent passwd $USERNAME | cut -d: -f6 ) | |
else | |
user_home="/home/${USERNAME}" | |
if [ ! -d "${user_home}" ]; then | |
mkdir -p "${user_home}" | |
chown ${USERNAME}:${group_name} "${user_home}" | |
fi | |
fi |
src/node/install.sh
Outdated
@@ -232,6 +232,8 @@ else | |||
[ ! -z "$https_proxy" ] && npm set https-proxy="$https_proxy" | |||
[ ! -z "$no_proxy" ] && npm set noproxy="$no_proxy" | |||
npm install -g pnpm | |||
pnpm setup |
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.
Thanks for the explanation.
Can you add few tests to validate your changes?
Runs the
pnpm setup
command, side effects of that command can be found here: https://pnpm.io/cli/setup