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

fix(npm): use correct flags and update version for npm #5533

Merged
merged 5 commits into from
Sep 6, 2022

Conversation

jsjoeio
Copy link
Contributor

@jsjoeio jsjoeio commented Sep 2, 2022

  • fix: update npm-postinstall.sh script
  • fix: use npm in release-standalone
  • chore: update package.json

@jsjoeio jsjoeio requested a review from a team as a code owner September 2, 2022 21:00
@jsjoeio jsjoeio changed the title release/v4.6.1 1 fix(npm): use correct flags and update version for npm Sep 2, 2022
@jsjoeio jsjoeio self-assigned this Sep 2, 2022
@@ -28,7 +28,7 @@ main() {
ln -s "./lib/node" "$RELEASE_PATH/node"

pushd "$RELEASE_PATH"
yarn --production --frozen-lockfile
npm install --unsafe-perm --omit=dev
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That'll help us catch this bug in the future.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I didn't get to do this yet... It was on my to-do list from #5071 (comment) but hadn't gotten to it yet...

Copy link
Contributor Author

@jsjoeio jsjoeio Sep 2, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh don't worry! All good 👍🏼 We didn't realize it would cause other issues so no worries at all. At least it's only npm!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I vaguely recall some issue with npm ci and optional dependencies; does that ring any bells?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think those were fixed with the latest versions of NPM, namely NPM 8 which we have since we bumped to NodeJS 16?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh gotcha!

@jsjoeio jsjoeio temporarily deployed to npm September 2, 2022 22:32 Inactive
@codecov
Copy link

codecov bot commented Sep 2, 2022

Codecov Report

Merging #5533 (0eb89b9) into main (6742e94) will not change coverage.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #5533   +/-   ##
=======================================
  Coverage   72.44%   72.44%           
=======================================
  Files          30       30           
  Lines        1673     1673           
  Branches      366      366           
=======================================
  Hits         1212     1212           
  Misses        398      398           
  Partials       63       63           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6742e94...0eb89b9. Read the comment docs.

@github-actions
Copy link

github-actions bot commented Sep 2, 2022

✨ code-server dev build published to npm for PR #5533!

  • Last publish status: success
  • Commit: 0eb89b9

To install in a local project, run:

npm install @coder/code-server-pr@5533

To install globally, run:

npm install -g @coder/code-server-pr@5533

@@ -28,7 +28,7 @@ main() {
ln -s "./lib/node" "$RELEASE_PATH/node"

pushd "$RELEASE_PATH"
yarn --production --frozen-lockfile
npm install --unsafe-perm --omit=dev
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I didn't get to do this yet... It was on my to-do list from #5071 (comment) but hadn't gotten to it yet...

@@ -140,7 +140,7 @@ install_with_yarn_or_npm() {
echo "yarn.lock file present, running in development mode. use yarn to install code-server!"
exit 1
else
npm install --omit=dev
npm install --unsafe-perm --legacy-peer-deps --omit=dev
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
npm install --unsafe-perm --legacy-peer-deps --omit=dev
# HACK: NPM's use of semver doesn't like resolving some peerDependencies that vscode (upstream) brings in the form of pre-releases.
# The legacy behavior doesn't complain about pre-releases being used, falling back to that for now.
# See https://github.com/coder/code-server/pull/5071
npm install --unsafe-perm --legacy-peer-deps --omit=dev

@@ -28,7 +28,7 @@ main() {
ln -s "./lib/node" "$RELEASE_PATH/node"

pushd "$RELEASE_PATH"
yarn --production --frozen-lockfile
npm install --unsafe-perm --omit=dev
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
npm install --unsafe-perm --omit=dev
npm ci --unsafe-perm --omit=dev

npm ci is the equivalent of yarn --frozen-lockfile I believe?

@edvincent
Copy link
Contributor

Had the same failure describe in #5530 after installing the with npm install -g @coder/code-server-pr@4.6.1-1-5533-8a151bd6cb529b3df463050920a16389b0b66453 --unsafe-perm 😬

But cd'ing to lib/vscode and running the command with the --legacy-peer-deps flag did work... Not sure what's up.

@jsjoeio
Copy link
Contributor Author

jsjoeio commented Sep 2, 2022

Nice suggestions! Since it's late afternoon Friday, I think we're going to make those in a followup PR so that we can get these artifacts and fix on npm asap

@edvincent
Copy link
Contributor

edvincent commented Sep 2, 2022

Ok, npm install -g @coder/code-server-pr@4.6.1-1-5533-8a151bd6cb529b3df463050920a16389b0b66453 --unsafe-perm actually works fine. I was following https://coder.com/docs/code-server/latest/npm#ubuntu-debian which doesn't (anymore) specify that libsecret-1-dev should be installed - so it was failing, but not because of the xterm problem.

So all good!

Also, NPM 8 tip: the logging of nested scripts needs the --foreground-scripts flag to be passed to be able to see the logs of nested postinstalls. See npm doc

(Which BTW, libsecret-1-dev is technically not required, because even though keytar is a required dependency, vscode soft-fails - i.e. fallbacks to an in-memory secret store - if it can't find keytar)

@ravi0the0sun
Copy link

Ok, npm install -g @coder/code-server-pr@4.6.1-1-5533-8a151bd6cb529b3df463050920a16389b0b66453 --unsafe-perm actually works fine. I was following https://coder.com/docs/code-server/latest/npm#ubuntu-debian which doesn't (anymore) specify that libsecret-1-dev should be installed - so it was failing, but not because of the xterm problem.

So all good!

Also, NPM 8 tip: the logging of nested scripts needs the --foreground-scripts flag to be passed to be able to see the logs of nested postinstalls. See npm doc

(Which BTW, libsecret-1-dev is technically not required, because even though keytar is a required dependency, vscode soft-fails - i.e. fallbacks to an in-memory secret store - if it can't find keytar)

Bad new for me the version of the code-server also gives the same error on my side. I'm using a raspberry pi over ipad usb3.

@edvincent
Copy link
Contributor

@ravi0the0sun If you install using --foreground-scripts, any ERR logs show up?

@ravi0the0sun
Copy link

ravi0the0sun commented Sep 3, 2022

@ravi0the0sun If you install using --foreground-scripts, any ERR logs show up?

Package libsecret-1 was not found in the pkg-config search path.yNode:node_modules/@coder/code-server-pr/lib/vscode/node_modules/@microsoft/1ds-core-js Completed in 2876ms
Perhaps you should add the directory containing `libsecret-1.pc'
to the PKG_CONFIG_PATH environment variable
No package 'libsecret-1' found
gyp: Call to 'pkg-config --cflags libsecret-1' returned exit status 1 while in binding.gyp. while trying to load binding.gyp
gyp ERR! configure error ify:pem: timing reifyNode:node_modules/@coder/code-server-pr/node_modules/ast-types Completed in 2283ms
gyp ERR! stack Error: `gyp` failed with exit code: 1
gyp ERR! stack     at ChildProcess.onCpExit (/usr/lib/node_modules/npm/node_modules/node-gyp/lib/configure.js:261:16)
gyp ERR! stack     at ChildProcess.emit (node:events:513:28)
gyp ERR! stack     at Process.ChildProcess._handle.onexit (node:internal/child_process:291:12)
 ERR! System Linux 5.15.61-v8+y:@microsoft/1ds-post-js: timing reifyNode:node_modules/@coder/code-server-pr/lib/vscode/node_modules/@microsoft/1ds-core-js Completed in 2876ms
gyp ERR! command "/usr/bin/node" "/usr/lib/node_modules/npm/node_modules/node-gyp/bin/node-gyp.js" "rebuild"
gyp ERR! cwd /usr/lib/node_modules/@coder/code-server-pr/lib/vscode/node_modules/keytar
gyp ERR! node -v v16.17.0
gyp ERR! node-gyp -v v9.0.0
gyp ERR! not ok 
npm ERR! code 1
npm ERR! path /usr/lib/node_modules/@coder/code-server-pr/lib/vscode/node_modules/keytar
npm ERR! command failed
npm ERR! command sh /tmp/install-2acda67b.sh

npm ERR! A complete log of this run can be found in:
npm ERR!     /root/.npm/_logs/2022-09-03T09_01_13_943Z-debug-0.log

im just posting the error part cuz the entire thing was too damn big i hope this helps

Update: after installing sudo apt install libsecret-1-dev it seems to work now
Thnaks all for your help

@jsjoeio jsjoeio added this to the September 2022 milestone Sep 6, 2022
@jsjoeio jsjoeio temporarily deployed to npm September 6, 2022 16:47 Inactive
@jsjoeio jsjoeio temporarily deployed to npm September 6, 2022 17:39 Inactive
@jsjoeio jsjoeio merged commit da03a64 into main Sep 6, 2022
@jsjoeio jsjoeio deleted the release/v4.6.1-1 branch September 6, 2022 18:03
@jsjoeio
Copy link
Contributor Author

jsjoeio commented Sep 6, 2022

@edvincent i will make your changes in a followup PR and tag you for review

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.

4 participants