[GitHub] [pulsar-client-node] shibd commented on a diff in pull request #288: Make it compatible with GLIBCXX_3.4.19 (CentOS 7)
shibd commented on code in PR #288: URL: https://github.com/apache/pulsar-client-node/pull/288#discussion_r1102532319 ## tests/load-test.sh: ## @@ -0,0 +1,42 @@ +#!/bin/bash Review Comment: Well, In short. My point is to verify `pulsar.node` does not belong to the unit test. Although it is important, it is part of the compilation. Let's go on and stop tangling. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@pulsar.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [pulsar-client-node] shibd commented on a diff in pull request #288: Make it compatible with GLIBCXX_3.4.19 (CentOS 7)
shibd commented on code in PR #288: URL: https://github.com/apache/pulsar-client-node/pull/288#discussion_r1099847687 ## tests/load-test.sh: ## @@ -0,0 +1,42 @@ +#!/bin/bash Review Comment: > What makes pkg directory so different that it can contain everything? For **test** binary file. It is a compile-time step. It may be more appropriately called `verify-xxx` - `./pkg/load_test.sh` -> `./pkg/verify-napi.sh` - `./tests/load-test.sh` and `./tests/docker-load-test.sh` -> `./pkg/linux/verify-multi-platforms-napi.sh` and `./pkg/linux/verify-multi-platfroms-napi-inside-docker.sh` What do you think of this? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@pulsar.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [pulsar-client-node] shibd commented on a diff in pull request #288: Make it compatible with GLIBCXX_3.4.19 (CentOS 7)
shibd commented on code in PR #288: URL: https://github.com/apache/pulsar-client-node/pull/288#discussion_r1099596237 ## tests/load-test.sh: ## @@ -0,0 +1,42 @@ +#!/bin/bash Review Comment: > Actually load_test.js should not be named as "xxx_test.js". It does not "test" anything. It's just a very trivial example script that creates a Client object and closes it. Yes, It is to test whether the binary package is usable, not unit testing. Similarly, `./tests/load-test.sh` and `docker-load-test.sh` also test whether the binary package can be used(Multi-platform under Linux). They are all part of the compilation process, so I recommend putting them in the `pkg` directory IMO, That only unit test-related files should be placed in the `tests` directory. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@pulsar.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [pulsar-client-node] shibd commented on a diff in pull request #288: Make it compatible with GLIBCXX_3.4.19 (CentOS 7)
shibd commented on code in PR #288: URL: https://github.com/apache/pulsar-client-node/pull/288#discussion_r1098170694 ## tests/load-test.sh: ## @@ -0,0 +1,42 @@ +#!/bin/bash Review Comment: @BewareMyPower Can you take a look at it? We want to release this fix to 1.8.1 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@pulsar.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [pulsar-client-node] shibd commented on a diff in pull request #288: Make it compatible with GLIBCXX_3.4.19 (CentOS 7)
shibd commented on code in PR #288: URL: https://github.com/apache/pulsar-client-node/pull/288#discussion_r1096914611 ## tests/load-test.sh: ## @@ -0,0 +1,42 @@ +#!/bin/bash Review Comment: Let's try to summarize these scripts: - `./tests/run-unit-test.sh`: Used as unit test verification for running CI, suitable for Linux platforms. So, put it in the `/tests` directory. - `./pkg/load_test.sh`: Used as a validation binary when running CI, suitable for all platforms(Require compile first). So, put it in the `./pkg/` dir. - `./tests/load-test.sh` and `./tests/docker-load-test.sh`:Used as validation binary on multiple platforms in Linux(Require compile first). I think `./tests/load-test.sh` and `./tests/docker-load-test.sh` is used to validate multiple platforms and are not part of unit testing. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@pulsar.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [pulsar-client-node] shibd commented on a diff in pull request #288: Make it compatible with GLIBCXX_3.4.19 (CentOS 7)
shibd commented on code in PR #288: URL: https://github.com/apache/pulsar-client-node/pull/288#discussion_r1072415206 ## .github/workflows/ci-pr-validation.yml: ## @@ -167,6 +167,14 @@ jobs: docker run -i -v $PWD:/pulsar-client-node build:latest \ /pulsar-client-node/pkg/linux/build-napi-inside-docker.sh + - name: Test NAPI file in other containers +if: matrix.image == 'linux_glibc' +run: | + ./tests/load-test.sh node:16-buster Review Comment: > The load-test.sh is used to test the Pulsar.node works for various versions of Node.js. I think that we should test on various types of operating systems(such as centos7, etc). But both use Node.js 18. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@pulsar.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [pulsar-client-node] shibd commented on a diff in pull request #288: Make it compatible with GLIBCXX_3.4.19 (CentOS 7)
shibd commented on code in PR #288:
URL:
https://github.com/apache/pulsar-client-node/pull/288#discussion_r1072407893
##
pkg/linux/Dockerfile_linux_glibc:
##
@@ -17,15 +17,19 @@
# under the License.
#
-ARG NODE_VERSION
+FROM centos:7
-FROM node:${NODE_VERSION}-buster
+RUN yum update -y
+RUN yum install -y gcc gcc-c++ make python3
-RUN apt-get update -y && \
- apt-get install -y \
-curl \
-g++ \
-make \
-python3
+ARG ARCH
+WORKDIR /app
+RUN SUFFIX=$(echo ${ARCH} | sed 's/86_//') && \
+ echo $SUFFIX && \
+ curl -O -L
https://nodejs.org/download/release/v16.19.0/node-v16.19.0-linux-$SUFFIX.tar.gz
&& \
Review Comment:
The GLIBCXX version is too high and is only related to the operating system
environment, right? It has nothing to do with the Node.js version, right?
The reason here is consistent with
[workflow](https://github.com/apache/pulsar-client-node/blob/f5fe0c23a4e6cffe9d781271ea08f181bf5bf6e7/.github/workflows/ci-pr-validation.yml#L135-L136)
is that the versions we provide to users are all compiled with the same node
version (Linux, windows, macOS).
BTW: The current TLS version of Node.js is 18.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [pulsar-client-node] shibd commented on a diff in pull request #288: Make it compatible with GLIBCXX_3.4.19 (CentOS 7)
shibd commented on code in PR #288: URL: https://github.com/apache/pulsar-client-node/pull/288#discussion_r1072396071 ## tests/load-test.sh: ## @@ -0,0 +1,42 @@ +#!/bin/bash Review Comment: Yes, But `docker-load-test` and `load-test.sh` are only used to test Linux-related in workflows. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@pulsar.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [pulsar-client-node] shibd commented on a diff in pull request #288: Make it compatible with GLIBCXX_3.4.19 (CentOS 7)
shibd commented on code in PR #288: URL: https://github.com/apache/pulsar-client-node/pull/288#discussion_r1072393932 ## tests/docker-load-test.sh: ## @@ -0,0 +1,35 @@ +#!/bin/bash +# +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +# + +# NOTE: This script should only run inside a Node.js docker container. +# See ./load-test.sh for details. +set -ex + +# Create an empty directory to test +mkdir -p /app && cd /app +tar zxf /pulsar-client-node/pulsar-client-node.tar.gz +cp /pulsar-client-node/build/Release/Pulsar.node . Review Comment: > If we're doing this way, we need to run npm install to install these dependencies This is ok, this is consistent with how users use. > And npm install might build the Pulsar.node again on the test docker image, so the Pulsar.node built on CentOS 7 might be overwritten. When `/pulsar-client-node/lib/binding/Pulsar.node`, It will no longer be compiled and overwritten. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@pulsar.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [pulsar-client-node] shibd commented on a diff in pull request #288: Make it compatible with GLIBCXX_3.4.19 (CentOS 7)
shibd commented on code in PR #288:
URL:
https://github.com/apache/pulsar-client-node/pull/288#discussion_r1071954273
##
tests/load-test.sh:
##
@@ -0,0 +1,42 @@
+#!/bin/bash
Review Comment:
This script is only used in Linux-related workflows and is recommended to be
placed in the `/pkg/linux/` directory
##
tests/docker-load-test.sh:
##
@@ -0,0 +1,35 @@
+#!/bin/bash
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+#
+
+# NOTE: This script should only run inside a Node.js docker container.
+# See ./load-test.sh for details.
+set -ex
+
+# Create an empty directory to test
+mkdir -p /app && cd /app
+tar zxf /pulsar-client-node/pulsar-client-node.tar.gz
+cp /pulsar-client-node/build/Release/Pulsar.node .
Review Comment:
You can just copy `Pulsar.node` to `/pulsar-client-node/lib/binding`
In this way, does not need to change its: `require()`
##
pkg/linux/Dockerfile_linux_glibc:
##
@@ -17,15 +17,19 @@
# under the License.
#
-ARG NODE_VERSION
+FROM centos:7
-FROM node:${NODE_VERSION}-buster
+RUN yum update -y
+RUN yum install -y gcc gcc-c++ make python3
-RUN apt-get update -y && \
- apt-get install -y \
-curl \
-g++ \
-make \
-python3
+ARG ARCH
+WORKDIR /app
+RUN SUFFIX=$(echo ${ARCH} | sed 's/86_//') && \
+ echo $SUFFIX && \
+ curl -O -L
https://nodejs.org/download/release/v16.19.0/node-v16.19.0-linux-$SUFFIX.tar.gz
&& \
Review Comment:
Here need to download the same version of Node.js as defined in
[workflow](https://github.com/apache/pulsar-client-node/blob/f5fe0c23a4e6cffe9d781271ea08f181bf5bf6e7/.github/workflows/ci-pr-validation.yml#L135-L136)
##
.github/workflows/ci-pr-validation.yml:
##
@@ -167,6 +167,14 @@ jobs:
docker run -i -v $PWD:/pulsar-client-node build:latest \
/pulsar-client-node/pkg/linux/build-napi-inside-docker.sh
+ - name: Test NAPI file in other containers
+if: matrix.image == 'linux_glibc'
+run: |
+ ./tests/load-test.sh node:16-buster
Review Comment:
Can we keep the param node version consistent with `${{matrix.nodejs}}`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
