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-4064 Update node library dependencies #1175

Closed
wants to merge 7 commits into from
Closed

THRIFT-4064 Update node library dependencies #1175

wants to merge 7 commits into from

Conversation

zertosh
Copy link

@zertosh zertosh commented Feb 1, 2017

  • The changes to node-int64 and Q are small:
  • ws under went a rewrite between 0.4.32 and 2.0.1 (websockets/ws@0.4.32...2.0.1). The API as used by thrift is backwards compatible. However, ws now uses ES6 syntax, so it requires at least Node 4.
    • Most notable change here is that ws no longer tries to install binary modules.
  • Node 4 is now the minimally required version of Node, due to ws' requirement.

@zertosh zertosh changed the title Update node library dependencies THRIFT-4064 Update node library dependencies Feb 1, 2017
Copy link
Contributor

@jeking3 jeking3 left a comment

Choose a reason for hiding this comment

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

@jfarrell says: will have to update the Dockerfile for debian to install a newer version so tests continue to pass

@zertosh
Copy link
Author

zertosh commented Feb 6, 2017

@jeking3 I don't really know what to change exactly in the Dockerfile :/

@zertosh
Copy link
Author

zertosh commented Feb 9, 2017

The tests are failing because run-browser's browserify can't find utf-8-validate (and it won't find bufferutil either. Those are optional dependencies of ws. You can't seem to configure run-browser to tell it to ignore those modules. So I'm going to try adding them to devDeps.

@jeking3
Copy link
Contributor

jeking3 commented Feb 9, 2017

I looked at the build results and in build job #2 it was still using node.js version 0.10 presumably because that's what is on the base install on the build slave being used:

NodeJS Library:
Using NodeJS .............. : /usr/bin/nodejs
Using NodeJS version....... : v0.10.29

The build failure has led me to a number of questions about how we maintain our docker images, who is responsible for that, and how we stage changes like this so CI builds can pass and prove the changes and new docker image are okay. I have sent emails out to others on the development team to learn this process and see if it needs to be modified to handle this type of change.

@jeking3
Copy link
Contributor

jeking3 commented Feb 10, 2017

I looked at the build failures. Build jobs 2 and 4 use the "debian" Dockerfile which needs to be updated in a very similar way to the ubuntu one. Give that a try. As for build jobs 3 and 8, I don't know exactly what happened but job #8 has me the most nervous since it was using the "ubuntu" image which we updated to node.js 4.7.2 and it looks like it hung during testing.

Copy link
Contributor

@jeking3 jeking3 left a comment

Choose a reason for hiding this comment

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

Build job 8 (https://travis-ci.org/apache/thrift/jobs/200386738) in Travis CI has hung twice now. This leads me to believe we have a bug or a test that isn't cleaning up properly. We could try going to 6.x and see if it makes a difference; otherwise this hang in the test needs to be sorted out before we commit.

@zertosh
Copy link
Author

zertosh commented Feb 11, 2017

@jeking3 pretty sure that the "SyntaxError: Parse error" error is phanthomjs (via run-browser) choking on the new syntax. The tests are running phantomjs@1.9.20 (https://travis-ci.org/apache/thrift/jobs/200386738#L2252), but it seems that it's v2 that added ES6 support (see ariya/phantomjs#14506).

Alternatively, how do you feel about making the require call for this module configurable?(

"var thrift = require('thrift');\n"
)

@jeking3
Copy link
Contributor

jeking3 commented Feb 11, 2017

I know very little about node.js in practice. It would be up to other committers/members/contributors who know node.js to weigh in on that. My biggest concern is the hang in build job 8. Can't merge unless we get a clean CI run, and ideally a knowledgeable code review from someone else. I'm happy to help in any way I can otherwise to help make progress.

@bgould
Copy link
Contributor

bgould commented Feb 11, 2017

@zertosh can you elaborate on

Alternatively, how do you feel about making the require call for this module configurable?(

"var thrift = require('thrift');\n"
)

Here's why I'm interested... I run the node library through browserify which works but by default produces a much larger glob of javascript than I really need, due to all of the thrift node components requiring that file which pulls in all of the other components whether I need them or not (eg I don't want to pull in TCompactProtocol or TFramedTransport etc if I'm just using TXHRTransport and TBinaryProtocol in the browser). At the moment I'm using some local edits to reduce what gets included by require('thrift') but IIRC the last time I looked at it, I came to the conclusion that the require('thrift') might not be needed at all (i.e., the modules that it aggregates can just be included explicitly and individually as needed). I would make code in the compiler possibly a bit more complicated but it could be worth the effort.

@zertosh
Copy link
Author

zertosh commented Feb 15, 2017

@bgould that's a very similar use case to ours. Also the Q library is kinda heavy, and could probably switch to just using native Promises.

For the purposes of this PR (avoid binary deps), I just switched to ws@1.x instead of ws@2.x, so that phantomjs doesn't choke. We'll still have some lib duplication, but at least you don't need to compile the dep.

@@ -113,8 +113,8 @@ RUN apt-get update && apt-get install -y --no-install-recommends \
neko-dev \
libneko0

# Node.js dependencies - THRIFT-4064 says it must be >= 4.x
RUN curl -sL https://deb.nodesource.com/setup_4.x | bash -
# Node.js dependencies - THRIFT-4064 says it must be >= 0.12.0
Copy link
Contributor

@jeking3 jeking3 Feb 15, 2017

Choose a reason for hiding this comment

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

Wasn't the whole point of this exercise to move up to node 4.x as a minimum supported version?

Copy link
Author

Choose a reason for hiding this comment

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

The point was to move away from ws@<1.0.0, since those versions depend on native node modules - they're a huge pain (#672 (comment)). Ideally we'd upgrade to the latest ws (v2.x), but the newer JS syntax is proving to be really problematic. Less ideally, but nonetheless solves the native module problem, is to upgrade to ws@^1.0.0. That only requires Node >= 0.12.0, and doesn't use newer syntax.

I still want to use ws@^2.0.0, but that requires other upstream dep fixes. I'm not really familiar with phantomjs, so that's going to take me a bit.

Copy link
Contributor

@jeking3 jeking3 left a comment

Choose a reason for hiding this comment

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

Given the discussion, looks like a step in the right direction, so approving.

@jeking3
Copy link
Contributor

jeking3 commented Feb 16, 2017

Looks like the bufferutil thing caused some build jobs to break.

@bgould
Copy link
Contributor

bgould commented Feb 16, 2017

`clang: error: unknown argument: '-fno-tree-vrp'

clang: error: unknown argument: '-fno-delete-null-pointer-checks'`

I'm guessing these are GCC-only options... and in that test runner CC=clang and CXX=clang++

I don't have time to look more into this right now but perhaps can experiment with it in the next few days

* The changes to node-int64 and Q are small:
  - broofa/node-int64@v0.3.3...v0.4.0
  - kriskowal/q@v1.0.1...v1.4.1
* ws under went a rewrite between 0.4.32 and 2.0.1 (websockets/ws@0.4.32...2.0.1). The API as used by thrift is backwards compatible. However, ws now uses ES6 syntax, so it requires at least Node 4.
  - Most notable change here is that ws no longer tries to install binary modules.
* Node 4 is now the minimally required version of Node, due to ws' requirement.
@zertosh
Copy link
Author

zertosh commented Mar 2, 2017

I'm at a loss here. I can't see this PR through. Sorry everyone, and thank you for your time.

@zertosh zertosh closed this Mar 2, 2017
@zertosh zertosh deleted the THRIFT-4064 branch March 2, 2017 04:14
@HiZhaoxiaoyang
Copy link

thrift... oh, that looks cool and kind of low level tool chain, maybe create a static typing language in the next century or a random url bring me here and give it a quick look.

"ws": "~0.4.32"
"node-int64": "^0.4.0",
"q": "^1.0.0",
"ws": "^1.0.0"
Copy link

@ledara1 ledara1 Sep 12, 2017

Choose a reason for hiding this comment

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

It should be >=1.1.1. In this version most of security issues were fixed. Please update the version.

Copy link
Contributor

Choose a reason for hiding this comment

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

@ledara1 I'm pretty sure this work was abandoned. We've since moved most of the build targets to more recent distributions, but this file wasn't updated. There is a new ubuntu-xenial docker image which, if you would be able to update the package.json file to match, would be useful.

@jeking3
Copy link
Contributor

jeking3 commented Sep 12, 2017

@zertosh We're on a new build environment based on ubuntu-xenial and it's more stable, perhaps you could resurrect your work here and complete it?

@zertosh
Copy link
Author

zertosh commented Sep 15, 2017

sorry @jeking3, I don't have the bandwidth these days to pick this back up

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants