You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@thrift.apache.org by zertosh <gi...@git.apache.org> on 2017/02/01 19:39:16 UTC
[GitHub] thrift pull request #1175: Update node library dependencies
GitHub user zertosh opened a pull request:
https://github.com/apache/thrift/pull/1175
Update node library dependencies
* The changes to node-int64 and Q are small:
- https://github.com/broofa/node-int64/compare/v0.3.3...v0.4.0
- https://github.com/kriskowal/q/compare/v1.0.1...v1.4.1
* ws under went a rewrite between 0.4.32 and 2.0.1 (https://github.com/websockets/ws/compare/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.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/zertosh/thrift THRIFT-4064
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/thrift/pull/1175.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #1175
----
commit face8c95163fc1b382ce7522b64334547fd43eca
Author: Andres Suarez <ze...@gmail.com>
Date: 2017-02-01T19:37:45Z
Update node library dependencies
* The changes to node-int64 and Q are small:
- https://github.com/broofa/node-int64/compare/v0.3.3...v0.4.0
- https://github.com/kriskowal/q/compare/v1.0.1...v1.4.1
* ws under went a rewrite between 0.4.32 and 2.0.1 (https://github.com/websockets/ws/compare/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.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1175: THRIFT-4064 Update node library dependencies
Posted by zertosh <gi...@git.apache.org>.
Github user zertosh commented on the issue:
https://github.com/apache/thrift/pull/1175
I'm at a loss here. I can't see this PR through. Sorry everyone, and thank you for your time.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1175: THRIFT-4064 Update node library dependencies
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1175#discussion_r101385004
--- Diff: build/docker/debian/Dockerfile ---
@@ -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
--- End diff --
Wasn't the whole pont of this exercise to move up to node 4.x as a minimum supported version?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1175: THRIFT-4064 Update node library dependencies
Posted by bgould <gi...@git.apache.org>.
Github user bgould commented on the issue:
https://github.com/apache/thrift/pull/1175
@zertosh can you elaborate on
> Alternatively, how do you feel about making the require call for this module configurable?(https://github.com/apache/thrift/blob/e1832c354391deb0e0ce94a62ff32e8ce1c83fd3/compiler/cpp/src/thrift/generate/t_js_generator.cc#L412)
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1175: THRIFT-4064 Update node library dependencies
Posted by zertosh <gi...@git.apache.org>.
Github user zertosh commented on the issue:
https://github.com/apache/thrift/pull/1175
@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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1175: THRIFT-4064 Update node library dependencies
Posted by zertosh <gi...@git.apache.org>.
Github user zertosh commented on the issue:
https://github.com/apache/thrift/pull/1175
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1175: THRIFT-4064 Update node library dependencies
Posted by zertosh <gi...@git.apache.org>.
Github user zertosh commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1175#discussion_r101392209
--- Diff: build/docker/debian/Dockerfile ---
@@ -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
--- End diff --
The point was to move away from `ws@<1.0.0`, since those versions depend on native node modules - they're a huge pain (https://github.com/apache/thrift/pull/672#issuecomment-276678791). 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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1175: THRIFT-4064 Update node library dependencies
Posted by ledara1 <gi...@git.apache.org>.
Github user ledara1 commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1175#discussion_r138406960
--- Diff: package.json ---
@@ -32,20 +32,22 @@
},
"main": "./lib/nodejs/lib/thrift",
"engines": {
- "node": ">= 0.2.4"
+ "node": ">= 0.12.0"
},
"dependencies": {
- "node-int64": "~0.3.0",
- "q": "1.0.x",
- "ws": "~0.4.32"
+ "node-int64": "^0.4.0",
+ "q": "^1.0.0",
+ "ws": "^1.0.0"
--- End diff --
It should be >=1.1.1. In this version most of security issues were fixed.
---
[GitHub] thrift pull request #1175: THRIFT-4064 Update node library dependencies
Posted by zertosh <gi...@git.apache.org>.
Github user zertosh closed the pull request at:
https://github.com/apache/thrift/pull/1175
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1175: THRIFT-4064 Update node library dependencies
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on the issue:
https://github.com/apache/thrift/pull/1175
@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?
---
[GitHub] thrift issue #1175: THRIFT-4064 Update node library dependencies
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on the issue:
https://github.com/apache/thrift/pull/1175
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1175: THRIFT-4064 Update node library dependencies
Posted by bgould <gi...@git.apache.org>.
Github user bgould commented on the issue:
https://github.com/apache/thrift/pull/1175
`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
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1175: THRIFT-4064 Update node library dependencies
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on the issue:
https://github.com/apache/thrift/pull/1175
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1175: THRIFT-4064 Update node library dependencies
Posted by zertosh <gi...@git.apache.org>.
Github user zertosh commented on the issue:
https://github.com/apache/thrift/pull/1175
@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 https://github.com/ariya/phantomjs/issues/14506).
Alternatively, how do you feel about making the `require` call for this module configurable?(https://github.com/apache/thrift/blob/e1832c354391deb0e0ce94a62ff32e8ce1c83fd3/compiler/cpp/src/thrift/generate/t_js_generator.cc#L412)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1175: THRIFT-4064 Update node library dependencies
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on the issue:
https://github.com/apache/thrift/pull/1175
Looks like the bufferutil thing caused some build jobs to break.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1175: THRIFT-4064 Update node library dependencies
Posted by zertosh <gi...@git.apache.org>.
Github user zertosh commented on the issue:
https://github.com/apache/thrift/pull/1175
sorry @jeking3, I don't have the bandwidth these days to pick this back up
---
[GitHub] thrift pull request #1175: THRIFT-4064 Update node library dependencies
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1175#discussion_r138415134
--- Diff: package.json ---
@@ -32,20 +32,22 @@
},
"main": "./lib/nodejs/lib/thrift",
"engines": {
- "node": ">= 0.2.4"
+ "node": ">= 0.12.0"
},
"dependencies": {
- "node-int64": "~0.3.0",
- "q": "1.0.x",
- "ws": "~0.4.32"
+ "node-int64": "^0.4.0",
+ "q": "^1.0.0",
+ "ws": "^1.0.0"
--- End diff --
@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.
---
[GitHub] thrift issue #1175: THRIFT-4064 Update node library dependencies
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on the issue:
https://github.com/apache/thrift/pull/1175
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1175: THRIFT-4064 Update node library dependencies
Posted by zertosh <gi...@git.apache.org>.
Github user zertosh commented on the issue:
https://github.com/apache/thrift/pull/1175
@jeking3 I don't really know what to change exactly in the Dockerfile :/
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1175: THRIFT-4064 Update node library dependencies
Posted by HiZhaoxiaoyang <gi...@git.apache.org>.
Github user HiZhaoxiaoyang commented on the issue:
https://github.com/apache/thrift/pull/1175
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---