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

[full] Update node version to LTS #178

Merged
merged 3 commits into from
Mar 30, 2020
Merged

[full] Update node version to LTS #178

merged 3 commits into from
Mar 30, 2020

Conversation

JesterOrNot
Copy link

Updates from 10.19.0 -> 12.16.1

@jankeromnes
Copy link
Contributor

@JesterOrNot Thanks a lot!

Please test this on Theia, because Theia is:

  • a big project that relies heavily on Node.js
  • an important project that we really don't want to break
  • a project that broke before due to a Node.js upgrade

To do so, please:

  1. fork https://github.com/eclipse-theia/theia
  2. change the fork's base image to gitpod/workspace-full-vnc:branch-sh-nvm
  3. open the fork in Gitpod, and verify that Theia builds successfully, that you're able to start Theia in a new preview, and that the development experience generally works great (i.e. the TypeScript language server, jump to definition, rename symbol, etc)

@JesterOrNot
Copy link
Author

@jankeromnes, Yea it breaks theia, where do we go from here, the way I see it if we become more and more out of date if we leave it.

@jankeromnes
Copy link
Contributor

Thanks a lot for testing it! Please request support for Node.js LTS by filing an issue against https://github.com/eclipse-theia/theia, and copy/paste the full error messages you're seeing with Node.js LTS.

I agree that we should get to a point where we always ship the latest stable version of each tool in workspace-full.

@JesterOrNot
Copy link
Author

JesterOrNot commented Mar 10, 2020

@akosyakov No, however something like

RUN bash -cl "echo -e \"\nnvm use 10\" >> ~/.bashrc"

Could maybe work in conjunction with the previous instruction

@jankeromnes
Copy link
Contributor

FYI the workspace-dotnet build failed with:

Too long with no output (exceeded 10m0s): context deadline exceeded

The fix is to increase that job's no-output time out, e.g. like so:

no_output_timeout: 30m

@jankeromnes
Copy link
Contributor

jankeromnes commented Mar 11, 2020

@JesterOrNot please rebase this Pull Request, and also rebase the test branch in your Theia fork and try this again.

There are new node / nvm fixes in both workspace-images and Theia that should help make this work.

@jankeromnes
Copy link
Contributor

I tried to test this again, but the nvm dir is still being forcefully added to the PATH:

gitpod /workspace/theia $ echo $PATH | sed 's_:_\n_g' | sort
/bin
/home/gitpod/.cargo/bin
/home/gitpod/.cargo/bin
/home/gitpod/.cargo/bin
/home/gitpod/go/bin
/home/gitpod/go-packages/bin
/home/gitpod/.nvm/versions/node/v12.16.1/bin
/home/gitpod/.pyenv/bin
/home/gitpod/.pyenv/plugins/pyenv-virtualenv/shims
/home/gitpod/.pyenv/plugins/pyenv-virtualenv/shims
/home/gitpod/.pyenv/shims
/home/gitpod/.pyenv/shims
/home/gitpod/.pyenv/shims
/home/gitpod/.rvm/bin
/home/gitpod/.rvm/bin
/home/gitpod/.rvm/bin
/home/gitpod/.rvm/gems/ruby-2.6.3/bin
/home/gitpod/.rvm/gems/ruby-2.6.3@global/bin
/home/gitpod/.rvm/rubies/ruby-2.6.3/bin
/home/gitpod/.sdkman/candidates/gradle/current/bin
/home/gitpod/.sdkman/candidates/java/current/bin
/home/gitpod/.sdkman/candidates/maven/current/bin
/home/linuxbrew/.linuxbrew/bin
/home/linuxbrew/.linuxbrew/sbin/
/sbin
/usr/bin
/usr/local/bin
/usr/local/sbin
/usr/sbin
/workspace/.cargo/bin
/workspace/.cargo/bin
/workspace/go/bin
/workspace/.pip-modules/bin
/workspace/.pip-modules/bin
/workspace/.rvm/bin
/workspace/.rvm/bin

Also, many directories are added twice. While this isn't a big issue, it's not super clean and potentially makes searching for binaries a fraction of a second slower.

@jankeromnes
Copy link
Contributor

But since the PATH force-add was removed in 6f17595, I think that the test is not relevant.

I.e., Gitpod is likely pulling an older gitpod/workspace-full-vnc:branch-sh-nvm version from its cache, so we're not actually testing the newest state of this PR. I'm not sure how to make Gitpod clear the Docker cache. Worst case we'll have to re-publish the newest image under a different tag (e.g. by opening a new PR).

@jankeromnes jankeromnes force-pushed the sh/nvm branch 3 times, most recently from 11733f0 to 24a1ebe Compare March 23, 2020 08:56
@jankeromnes
Copy link
Contributor

jankeromnes commented Mar 23, 2020

FYI, we've decided to keep the hard-coded Node.js version in the PATH for now, but upgrade to LTS.

The reason for keeping the hard-coded version is that the Node.js debugging extension requires which node to work without sourcing ~/.bashrc.

This way most projects will have a convenient setup that just works (no extra step required to get Node.js LTS). It will only get a little awkward for projects who need a different Node.js version, because they will need to add this to their .gitpod.Dockerfile:

FROM gitpod/workspace-full

# Install & use custom Node.js version
ENV NODE_VERSION=10.19.0
RUN bash -c ". .nvm/nvm.sh && \
        nvm install ${NODE_VERSION} && \
        nvm alias default ${NODE_VERSION}"`
ENV PATH=/home/gitpod/.nvm/versions/node/v${NODE_VERSION}/bin:$PATH

(Note that the new hard-coded Node.js binary path should be "in front of" (i.e. "left of") the pre-existing $PATH, otherwise the old hard-coded version will prevail.)

jankeromnes added a commit to jankeromnes/theia that referenced this pull request Mar 23, 2020
jankeromnes added a commit to jankeromnes/theia that referenced this pull request Mar 23, 2020
@jankeromnes
Copy link
Contributor

jankeromnes commented Mar 23, 2020

Tested with jankeromnes/theia@02b0463, it works well!

Required this Theia fix though: jankeromnes/theia@16d697f (which I'll need to contribute upstream)

@akosyakov
Copy link
Member

It will only get a little awkward for projects who need a different Node.js version, because they will need to add this to their Dockerfile:

@jankeromnes sounds like something worth to add to Gitpod docs

jankeromnes added a commit to jankeromnes/theia that referenced this pull request Mar 23, 2020
@jankeromnes
Copy link
Contributor

Required this Theia fix though: jankeromnes/theia@16d697f (which I'll need to contribute upstream)

Done in eclipse-theia/theia#7404

It will only get a little awkward for projects who need a different Node.js version, because they will need to add this to their Dockerfile:

@jankeromnes sounds like something worth to add to Gitpod docs

Hmm, true. While this is awkward and "temporary" until we find a better solution, it seems to be the current "official" way to change Node.js versions.

@nisarhassan12 Could you please mention the trick described in #178 (comment) about how to switch to a different Node.js version, in your upcoming "JavaScript in Gitpod" guide?

@nisarhassan12
Copy link

@jankeromnes Thanks! sure.

@jankeromnes
Copy link
Contributor

jankeromnes commented Mar 24, 2020

This seems to work very well!

Tomorrow, we are about to release a Gitpod.io update that will make the Theia IDE use its own Node.js v10 binary regardless of what Node.js version is used by default in the workspace.

So, assuming the deployment goes well and sticks, we will be able to merge this PR into master, e.g. on Thursday Friday. 🚀

@jankeromnes jankeromnes force-pushed the sh/nvm branch 5 times, most recently from bcc451a to e63e225 Compare March 30, 2020 10:20
@jankeromnes
Copy link
Contributor

Small CircleCI hiccup with the gitpod/workspace-dotnet image:

3d07dc885a78: Pushing  17.43GB/16.98GB

Too long with no output (exceeded 30m0s): context deadline exceeded

Should probably bump the timeout just in case.

jankeromnes added a commit to jankeromnes/theia that referenced this pull request Mar 30, 2020
@jankeromnes jankeromnes merged commit 24c0a0b into master Mar 30, 2020
@jankeromnes jankeromnes deleted the sh/nvm branch March 30, 2020 16:05
@JesterOrNot
Copy link
Author

Awesome!

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