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

Upgrades #32

Closed
wants to merge 2 commits into from
Closed

Upgrades #32

wants to merge 2 commits into from

Conversation

jankeromnes
Copy link
Contributor

There are new versions of Ruby and Node.js.

A word of caution: I didn't test this, so it might break things down the line.

@svenefftinge
Copy link
Member

Thanks for the contribution. Bumping node from 8 to 10 is not really a minor change.

@jankeromnes
Copy link
Contributor Author

jankeromnes commented Dec 11, 2018

Ok you're right. It used to be a breaking change for Janitor, but Node 10 backwards compatibility seems to have improved lately because we were recently able to upgrade all our images without further changes or breakage.

But it should definitely be tested first. How can I do that?

Edit: I did this very quick change just in case it was useful. Please feel free to drop it for now if it's inconvenient. It could also be picked up later.

@jankeromnes jankeromnes changed the title Minor upgrades Upgrades Dec 11, 2018
@svenefftinge
Copy link
Member

@akosyakov @AlexTugarev opinions?

@@ -70,7 +70,7 @@ RUN $HOME/.cargo/bin/rustup update \
&& $HOME/.cargo/bin/rustup component add rls-preview rust-analysis rust-src

# Install nvm with node and npm
ENV NODE_VERSION 8
ENV NODE_VERSION 10
Copy link
Member

Choose a reason for hiding this comment

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

Working on Theia requires node version 8. We'd need to create a custom image for Theia then, right?

Copy link
Contributor Author

@jankeromnes jankeromnes Dec 11, 2018

Choose a reason for hiding this comment

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

It's true that upgrading to node 10 used to break Theia's install, but this has now been fixed. Are there any other Theia / node 10 blockers that I'm not aware of? We should probably test this, and maybe take it upstream.

Copy link
Member

Choose a reason for hiding this comment

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

That's a good question. I've seen a lot of effort towards node 10 support, but I'm not sure how far this is.

I just did a quick test, and there is still something to to on the Theia side.

Copy link
Member

Choose a reason for hiding this comment

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

FYI there is a PR @kittaakos is working on to make Theia run with node 10.

Copy link
Member

Choose a reason for hiding this comment

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

+1 for bumping. We can handle it in Theia's .gitpod.yml

@orucky
Copy link
Contributor

orucky commented Dec 11, 2018

Isn't it that we have nvm there to let user use any version of Node.js he wants? In Theia (and other descending images) we can switch to some fixed version (currently 8.14.0, see #34), I'd also keep it there as a default in case someone wants to try Gitpod for opening the Theia project & build it, but we can preinstall Node 10 there too, so the user can switch fast. Am I missing something? (That's well possible)

@svenefftinge
Copy link
Member

The question is what should be set by default. It makes sense to use the latest stable which is Node 10.

@orucky
Copy link
Contributor

orucky commented Dec 12, 2018

One note - I bumped Ruby independently in #34. I know there's also a bit newer version of Python (3.6.7), Node.js is current (in 8.* line), there's Clang 7 instead of 6.0 (I'm creating an issue for this one as it's a major version). The rest I haven't checked

@jankeromnes
Copy link
Contributor Author

Thanks all! This pull request is now superseded by #69 and #70.

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