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

THRIFT-5116: Update NodeJS to supported version on Ubuntu Xenial #2032

Merged

Conversation

emmenlau
Copy link
Member

@emmenlau emmenlau commented Feb 24, 2020

This PR updates NodeJS on Ubuntu Xenial to 10.x. This is because NodeJS 6.x that was previously used does not support newer node modules. Also, since January 2020, NodeJS 8.x is EOL. Therefore 10.x is the currently oldest, supported NodeJS version.

The Dockerfile was tested locally so the docker build should work. Also, NodeJS is the currently successfully employed version on other thrift docker builds.

This PR fixes the issue https://travis-ci.org/apache/thrift/jobs/654070652 that is now solved in https://travis-ci.org/apache/thrift/jobs/654644828, so please review and merge. However note that the build still does not fully succeed with a later error about a required Ruby update. This second issue is very likely unrelated to this PR.

  • Apache Jira ticket THRIFT-5116
  • If a ticket exists: Does your pull request title follow the pattern "THRIFT-NNNN: describe my issue"?
  • Did you squash your changes to a single commit? (not required, but preferred)
  • Did you do your best to avoid breaking changes? If one was needed, did you label the Jira ticket with "Breaking-Change"?
  • If your change does not involve any code, add [skip ci] at the end of your pull request to free up build resources.

@Jens-G
Copy link
Member

Jens-G commented Mar 10, 2020

Is that related to / superseded by THRIFT-5120 maybe?

@emmenlau
Copy link
Member Author

I'm not really an expert, but I think this PR supersedes THRIFT-5120. THRIFT-5120 uses Node 8.x which since January 2020 is EOL. I think it may be better to go for the currently supported Node versions, or what do you think?

@dcelasun
Copy link
Member

Please also update LANGUAGES.md and CHANGES.md.

@emmenlau emmenlau force-pushed the bda_update_eol_node_on_xenial branch from 65f6ecb to 02f4a18 Compare April 24, 2020 07:59
@emmenlau
Copy link
Member Author

Thanks @dcelasun, fixed now. Please review.

@dcelasun
Copy link
Member

Unfortunately this seems to have broken Travis:

Rebuilding docker image ubuntu-xenial
Sending build context to Docker daemon 11.26kB
Error response from daemon: Dockerfile parse error line 60: unknown instruction: APT-ADD-REPOSITORY
The command "if [[ uname == "Linux" ]]; then build/docker/refresh.sh; fi" failed and exited with 1 during .

@emmenlau
Copy link
Member Author

Unfortunately this seems to have broken Travis:

Error response from daemon: Dockerfile parse error line 60: unknown instruction: APT-ADD-REPOSITORY
The command "if [[ uname == "Linux" ]]; then build/docker/refresh.sh; fi" failed and exited with 1 during .

Possibly you are right, but I've just seen a very similar error in an unrelated build. Could you pelase re-trigger Travis once to see if that helps? See also my related email to dev-list from today.

@dcelasun
Copy link
Member

dcelasun commented Apr 24, 2020

I've restarted it and it failed again, because there really is a bug 🙂 This:

echo "deb https://deb.nodesource.com/node_10.x xenial main" | tee /etc/apt/sources.list.d/nodesource.list

should be

echo "deb https://deb.nodesource.com/node_10.x xenial main" | tee /etc/apt/sources.list.d/nodesource.list && \

(note the && \ at the end)

@emmenlau emmenlau force-pushed the bda_update_eol_node_on_xenial branch from 02f4a18 to 5fc13f6 Compare April 24, 2020 11:31
@emmenlau
Copy link
Member Author

Dammit, you are right! Thanks.

Slightly off-topic, I think the comments in the Dockerfile should really be changed. I've had good experience with adding comments in chains of && via echo. So instead of

# dotnet (core)
    curl https://packages.microsoft.com/keys/microsoft.asc | gpg --dearmor > /etc/apt/trusted.gpg.d/microsoft.gpg && \
    echo "deb [arch=amd64] https://packages.microsoft.com/repos/microsoft-ubuntu-xenial-prod xenial main" > \
      /etc/apt/sources.list.d/dotnetdev.list && \

# node.js
    curl -sL https://deb.nodesource.com/gpgkey/nodesource.gpg.key | apt-key add - && \
    echo "deb https://deb.nodesource.com/node_10.x xenial main" | tee /etc/apt/sources.list.d/nodesource.list && \

# ruby 2.4
    apt-add-repository ppa:brightbox/ruby-ng

I personally would highly prefer

    echo "Installing dotnet (core)" && \
    curl https://packages.microsoft.com/keys/microsoft.asc | gpg --dearmor > /etc/apt/trusted.gpg.d/microsoft.gpg && \
    echo "deb [arch=amd64] https://packages.microsoft.com/repos/microsoft-ubuntu-xenial-prod xenial main" > \
      /etc/apt/sources.list.d/dotnetdev.list && \
    echo "Installing node.js" && \
    curl -sL https://deb.nodesource.com/gpgkey/nodesource.gpg.key | apt-key add - && \
    echo "deb https://deb.nodesource.com/node_10.x xenial main" | tee /etc/apt/sources.list.d/nodesource.list && \
    echo "Installing ruby 2.4" && \
    apt-add-repository ppa:brightbox/ruby-ng

This does some part of documentation, remains visible in the execution, and most importantly it does not break the chain of &&-connections.

@dcelasun
Copy link
Member

That's a great idea actually, please go ahead and change it.

@emmenlau emmenlau force-pushed the bda_update_eol_node_on_xenial branch 2 times, most recently from 21c80d7 to 0de155a Compare April 27, 2020 08:52
@emmenlau emmenlau force-pushed the bda_update_eol_node_on_xenial branch from 0de155a to 3e59a4b Compare May 11, 2020 19:21
@emmenlau
Copy link
Member Author

Pushed again to re-trigger the CI. Please consider for merging if the build now succeeds?

@dcelasun
Copy link
Member

Travis is all green, merged 🎉

@dcelasun dcelasun merged commit 55680af into apache:master May 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants