Skip to content

Conversation

jsjoeio
Copy link
Contributor

@jsjoeio jsjoeio commented Mar 26, 2021

Created with brew bump-formula-pr.

@BrewTestBot BrewTestBot added the nodejs Node or npm use is a significant feature of the PR or issue label Mar 26, 2021
@jsjoeio jsjoeio mentioned this pull request Mar 26, 2021
13 tasks
@carlocab
Copy link
Member

==> yarn --production --frozen-lockfile
yarn install v1.22.10
info No lockfile found.
[1/5] Validating package.json...
error [email protected]: The engine "node" is incompatible with this module. Expected version ">= 12 <= 14". Got "15.12.0"
error Found incompatible module.

@carlocab carlocab added the build failure CI fails while building the software label Mar 26, 2021
@jsjoeio
Copy link
Contributor Author

jsjoeio commented Mar 26, 2021

The engine "node" is incompatible with this module. Expected version ">= 12 <= 14". Got "15.12.0"

CI tests are using Node v15 but we only support 12-14. This seems like an expected failure, no?

We introduced a tighter restriction around Node requirements in 3.9.2 because when you're developing/contributing, there are issues with Node v15. This is based on us having to align with the requirements of VS Code.

AH - my teammate found this in your docs:

Node modules which are compatible with the latest Node version should declare a dependency on the node formula.

depends_on "node"

If your formula requires being executed with an older Node version you should use one of the versioned node formulae (e.g. node@12).

I wonder if it would be possible to echo that in CI and link to the docs - it would definitely be helpful for folks newer to the homebrew ecosystem!

@jsjoeio jsjoeio marked this pull request as draft March 26, 2021 21:42
We introduced a tighter restriction around Node requirements in 3.9.2 because when you're developing/contributing, there are issues with Node v15. This is based on us having to align with the requirements of VS Code.
@jsjoeio jsjoeio marked this pull request as ready for review March 26, 2021 21:49
@jsjoeio
Copy link
Contributor Author

jsjoeio commented Mar 26, 2021

==> /opt/homebrew/Cellar/code-server/3.9.2/bin/code-server --extensions-dir=. --install-extension ms-python.python
env: node: No such file or directory

Hmm...investigating

@jsjoeio jsjoeio marked this pull request as draft March 26, 2021 21:59
This updates the URL in the comment.

This uses a basic theme extension because it doesn't update often 
and is more reliable for testing.
@jsjoeio jsjoeio marked this pull request as ready for review March 26, 2021 22:03
jsjoeio and others added 2 commits March 26, 2021 15:05
Co-authored-by: Carlo Cabrera <[email protected]>
Co-authored-by: Carlo Cabrera <[email protected]>
@carlocab
Copy link
Member

I wonder if it would be possible to echo that in CI and link to the docs

Not sure how CI would know when to link the docs -- it just prints out build errors but doesn't parse them. However, maybe there's a solution here I'm just not thinking of. PRs to improve things appreciated.

@carlocab carlocab removed the build failure CI fails while building the software label Mar 26, 2021
@jsjoeio
Copy link
Contributor Author

jsjoeio commented Mar 26, 2021

==> /opt/homebrew/Cellar/code-server/3.9.2/bin/code-server --extensions-dir=. --install-extension wesbos.theme-cobalt2
env: node: No such file or directory

Not sure why that's failing... going to check to make sure it's working for me locally (by installing from npm)

@carlocab
Copy link
Member

carlocab commented Mar 26, 2021

Your issue might be that the versioned Node formulae are keg-only.

Is there an environment variable that code-server checks for the path to a Node executable? If so, we should wrap the binary in an env script that sets that for the user. Or an env script that prepends to the user's PATH appropriately, but I'm less fond of that option.

Edit: Oh, wait. The formula already does that. Except that it prepends the wrong path in the env script.

@jsjoeio
Copy link
Contributor Author

jsjoeio commented Mar 26, 2021

Yeah I tested locally and it's working as expected so it's not the test.

image

Ahh, hmm... thinking.

@jsjoeio
Copy link
Contributor Author

jsjoeio commented Mar 26, 2021

Except that it prepends the wrong path in the env script

What is the right path?

I'm having trouble finding this in the docs. This was passed on from a previous maintainer, and as you can see, we need to add more documentation for current maintainers.

@carlocab
Copy link
Member

Try this:

diff --git a/Formula/code-server.rb b/Formula/code-server.rb
index 58b528c1654..fbc2313c6bf 100644
--- a/Formula/code-server.rb
+++ b/Formula/code-server.rb
@@ -24,9 +24,12 @@ class CodeServer < Formula
   end
 
   def install
+    node = Formula["node@14"]
+
     system "yarn", "--production", "--frozen-lockfile"
     libexec.install Dir["*"]
-    env = { PATH: "#{HOMEBREW_PREFIX}/opt/node/bin:$PATH" }
+
+    env = { PATH: "#{node.opt_bin}:$PATH" }
     (bin/"code-server").write_env_script "#{libexec}/out/node/entry.js", env
   end

You could've probably just done s/node/node@14 in the line that defines env, but I'm thinking this makes it a bit more obvious that something else needs to be changed if the Node dependency changes again in the future.

@jsjoeio
Copy link
Contributor Author

jsjoeio commented Mar 26, 2021

Ah! Thank you! 🙏 As you can probably tell, I haven't written Ruby before.

We'll make sure to update the docs in code-server to explain this as well (should it need to be updated in the future).

@jsjoeio
Copy link
Contributor Author

jsjoeio commented Mar 26, 2021

brew bot shamming me for editing in GitHub web instead of in my local editor

image

@jsjoeio jsjoeio marked this pull request as draft March 26, 2021 23:07
@jsjoeio jsjoeio marked this pull request as ready for review March 26, 2021 23:08
Copy link
Member

@Rylan12 Rylan12 left a comment

Choose a reason for hiding this comment

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

Looks good! Thanks, @jsjoeio!

@BrewTestBot
Copy link
Member

🤖 A scheduled task has triggered a merge.

@github-actions github-actions bot added the outdated PR was locked due to age label Apr 27, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 27, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
nodejs Node or npm use is a significant feature of the PR or issue outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants