You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2022/10/27 08:27:10 UTC

[GitHub] [pulsar-client-node] shibd opened a new pull request, #238: [feat] Windows support build package c++ static binary.

shibd opened a new pull request, #238:
URL: https://github.com/apache/pulsar-client-node/pull/238

   ### Motivation
   
   #235 
   
   Support windows build of static link c++ lib.
   
   ### Modifications
   - Download the C++ static library directly instead of recompiling
   
   PS: Since there is no static library release for C++ yet, Use a [temporary release](https://github.com/BewareMyPower/pulsar-client-cpp/releases/tag/v3.1.0-rc-20221026-windows) temporarily. 
   
   ### Documentation
   
   - [ ] `doc` <!-- Your PR contains doc changes. Please attach the local preview screenshots (run `sh start.sh` at `pulsar/site2/website`) to your PR description, or else your PR might not get merged. -->
   - [x] `doc-required` <!-- Your PR changes impact docs and you will update later -->
   - [ ] `doc-not-needed` <!-- Your PR changes do not impact docs -->
   - [ ] `doc-complete` <!-- Docs have been already added -->
   
   
   
   


-- 
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-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [pulsar-client-node] shibd commented on pull request #238: [feat] Support windows build napi of static link c++ lib.

Posted by GitBox <gi...@apache.org>.
shibd commented on PR #238:
URL: https://github.com/apache/pulsar-client-node/pull/238#issuecomment-1296909950

   > ![image](https://user-images.githubusercontent.com/18204803/198980261-5b4789b7-407a-43a5-9da6-138161435699.png)
   > 
   > You can see the PR validation only verified the case of x86 Windows and Node.js 16. The Node.js version compatibility checks should be performed when pushing a PR, not when pushing a tag.
   
   
   
   > ![image](https://user-images.githubusercontent.com/18204803/198980261-5b4789b7-407a-43a5-9da6-138161435699.png)
   > 
   > You can see the PR validation only verified the case of x86 Windows and Node.js 16. The Node.js version compatibility checks should be performed when pushing a PR, not when pushing a tag.
   
   Yes, I added more verification conditions.


-- 
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-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [pulsar-client-node] shibd commented on a diff in pull request #238: [feat] Support windows build napi of static link c++ lib.

Posted by GitBox <gi...@apache.org>.
shibd commented on code in PR #238:
URL: https://github.com/apache/pulsar-client-node/pull/238#discussion_r1009274759


##########
.github/workflows/ci-build-release-napi.yml:
##########
@@ -125,3 +125,68 @@ jobs:
         with:
           name: ${{matrix.image}}-${{matrix.nodejs}}-${{matrix.cpu.platform}}
           path: build/stage/*/*.tar.gz
+
+  windows-napi:
+    name: Build NAPI windows - Node ${{matrix.nodejs}} - ${{matrix.arch}}
+    runs-on: windows-2022
+    timeout-minutes: 3000
+
+    strategy:
+      fail-fast: false
+      matrix:
+        arch:
+          - x64
+          - x86
+        nodejs:
+          - 16
+          - 18
+    steps:
+      - uses: actions/checkout@v3
+      - name: Use Node.js ${{ matrix.nodejs }}
+        uses: actions/setup-node@v3
+        with:
+          node-version: ${{ matrix.nodejs }}
+          architecture: ${{ matrix.arch }}
+          cache: 'npm'
+
+      - name: Cache CPP Client
+        id: cache-dependencies
+        uses: actions/cache@v3
+        with:
+          path: pkg/windows/
+          key: ${{ runner.os }}-${{ matrix.arch }}-${{ hashFiles('tmp-pulsar-client-cpp-version.txt') }}
+
+      - name: Add env vars
+        shell: bash
+        run: |
+          if [ "${{ matrix.arch }}" = "x86" ]; then
+            echo "TARGET=ia32" >> $GITHUB_ENV
+          else
+            echo "TARGET=${{ matrix.arch }}" >> $GITHUB_ENV
+          fi
+
+      - name: Download CPP Client
+        if: steps.cache-dependencies.outputs.cache-hit != 'true'
+        run: |
+          pkg/windows/download-cpp-client.bat ${{ matrix.arch }}
+
+      - name: Build Node binaries lib
+        run: |
+          npm install --ignore-scripts
+          npx node-pre-gyp configure --target_arch=${{ env.TARGET }}
+          npx node-pre-gyp build --target_arch=${{ env.TARGET }}
+
+      - name: Test loading Node binaries lib
+        run: |
+          node pkg/load_test.js
+
+      - name: Package Node binaries lib
+        run: |
+          npx node-pre-gyp package --target_arch=${{ env.TARGET }}
+
+      - name: Upload artifacts
+        uses: actions/upload-artifact@v3
+        if: matrix.nodejs == 16

Review Comment:
   I thought about it, based on the [compatibility guarantee of `NAPI` and `Node.js`](https://nodejs.org/api/n-api.html#node-api-version-matrix). We no longer need to verify the versions of both `NAPI` and `Node.js`. I removed the node 16 parameter



-- 
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-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [pulsar-client-node] shibd commented on pull request #238: [feat] Support windows build napi of static link c++ lib.

Posted by GitBox <gi...@apache.org>.
shibd commented on PR #238:
URL: https://github.com/apache/pulsar-client-node/pull/238#issuecomment-1297885027

   > Is there any way to verify the node module built with NPI of Node.js 18 can be used by Node.js 16?
   
   I guess no verification is needed, We built using version `4` of NAPI.
   
   https://github.com/apache/pulsar-client-node/blob/10ef71a0d235955d91bf763d12513eca51948df0/binding.gyp#L37
   
   Then refer to this [table](https://nodejs.org/api/n-api.html#implications-of-abi-stability) to know that both 18 and 16 can be used.
   
   


-- 
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-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [pulsar-client-node] BewareMyPower commented on a diff in pull request #238: [feat] Support windows build napi of static link c++ lib.

Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on code in PR #238:
URL: https://github.com/apache/pulsar-client-node/pull/238#discussion_r1009221182


##########
.github/workflows/ci-build-release-napi.yml:
##########
@@ -125,3 +125,68 @@ jobs:
         with:
           name: ${{matrix.image}}-${{matrix.nodejs}}-${{matrix.cpu.platform}}
           path: build/stage/*/*.tar.gz
+
+  windows-napi:
+    name: Build NAPI windows - Node ${{matrix.nodejs}} - ${{matrix.arch}}
+    runs-on: windows-2022
+    timeout-minutes: 3000
+
+    strategy:
+      fail-fast: false
+      matrix:
+        arch:
+          - x64
+          - x86
+        nodejs:
+          - 16
+          - 18
+    steps:
+      - uses: actions/checkout@v3
+      - name: Use Node.js ${{ matrix.nodejs }}
+        uses: actions/setup-node@v3
+        with:
+          node-version: ${{ matrix.nodejs }}
+          architecture: ${{ matrix.arch }}
+          cache: 'npm'
+
+      - name: Cache CPP Client
+        id: cache-dependencies
+        uses: actions/cache@v3
+        with:
+          path: pkg/windows/
+          key: ${{ runner.os }}-${{ matrix.arch }}-${{ hashFiles('tmp-pulsar-client-cpp-version.txt') }}
+
+      - name: Add env vars
+        shell: bash
+        run: |
+          if [ "${{ matrix.arch }}" = "x86" ]; then
+            echo "TARGET=ia32" >> $GITHUB_ENV
+          else
+            echo "TARGET=${{ matrix.arch }}" >> $GITHUB_ENV
+          fi
+
+      - name: Download CPP Client
+        if: steps.cache-dependencies.outputs.cache-hit != 'true'
+        run: |
+          pkg/windows/download-cpp-client.bat ${{ matrix.arch }}
+
+      - name: Build Node binaries lib
+        run: |
+          npm install --ignore-scripts
+          npx node-pre-gyp configure --target_arch=${{ env.TARGET }}
+          npx node-pre-gyp build --target_arch=${{ env.TARGET }}
+
+      - name: Test loading Node binaries lib
+        run: |
+          node pkg/load_test.js
+
+      - name: Package Node binaries lib
+        run: |
+          npx node-pre-gyp package --target_arch=${{ env.TARGET }}
+
+      - name: Upload artifacts
+        uses: actions/upload-artifact@v3
+        if: matrix.nodejs == 16

Review Comment:
   But this is `ci-build-release-napi.yml`, which is used to build the artifacts, rather than validation. If you want to verify the source code can be compiled with different versions of Node.js, you should add the workflows into `ci-pr-validation.yml`.



-- 
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-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [pulsar-client-node] shibd commented on a diff in pull request #238: [feat] Support windows build napi of static link c++ lib.

Posted by GitBox <gi...@apache.org>.
shibd commented on code in PR #238:
URL: https://github.com/apache/pulsar-client-node/pull/238#discussion_r1009144855


##########
.github/workflows/ci-build-release-napi.yml:
##########
@@ -125,3 +125,68 @@ jobs:
         with:
           name: ${{matrix.image}}-${{matrix.nodejs}}-${{matrix.cpu.platform}}
           path: build/stage/*/*.tar.gz
+
+  windows-napi:
+    name: Build NAPI windows - Node ${{matrix.nodejs}} - ${{matrix.arch}}
+    runs-on: windows-2022
+    timeout-minutes: 3000
+
+    strategy:
+      fail-fast: false
+      matrix:
+        arch:
+          - x64
+          - x86
+        nodejs:
+          - 16
+          - 18
+    steps:
+      - uses: actions/checkout@v3
+      - name: Use Node.js ${{ matrix.nodejs }}
+        uses: actions/setup-node@v3
+        with:
+          node-version: ${{ matrix.nodejs }}
+          architecture: ${{ matrix.arch }}
+          cache: 'npm'
+
+      - name: Cache CPP Client
+        id: cache-dependencies
+        uses: actions/cache@v3
+        with:
+          path: pkg/windows/
+          key: ${{ runner.os }}-${{ matrix.arch }}-${{ hashFiles('tmp-pulsar-client-cpp-version.txt') }}
+
+      - name: Add env vars
+        shell: bash
+        run: |
+          if [ "${{ matrix.arch }}" = "x86" ]; then
+            echo "TARGET=ia32" >> $GITHUB_ENV
+          else
+            echo "TARGET=${{ matrix.arch }}" >> $GITHUB_ENV
+          fi
+
+      - name: Download CPP Client
+        if: steps.cache-dependencies.outputs.cache-hit != 'true'
+        run: |
+          pkg/windows/download-cpp-client.bat ${{ matrix.arch }}
+
+      - name: Build Node binaries lib
+        run: |
+          npm install --ignore-scripts
+          npx node-pre-gyp configure --target_arch=${{ env.TARGET }}
+          npx node-pre-gyp build --target_arch=${{ env.TARGET }}
+
+      - name: Test loading Node binaries lib
+        run: |
+          node pkg/load_test.js
+
+      - name: Package Node binaries lib
+        run: |
+          npx node-pre-gyp package --target_arch=${{ env.TARGET }}
+
+      - name: Upload artifacts
+        uses: actions/upload-artifact@v3
+        if: matrix.nodejs == 16

Review Comment:
   Oh, While I was coding. Node.js 16 is the TLS version. Now its TLS changed to 18. 
   
   https://nodejs.org/en/



-- 
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-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [pulsar-client-node] shibd commented on a diff in pull request #238: [feat] Support windows build napi of static link c++ lib.

Posted by GitBox <gi...@apache.org>.
shibd commented on code in PR #238:
URL: https://github.com/apache/pulsar-client-node/pull/238#discussion_r1009163207


##########
.github/workflows/ci-build-release-napi.yml:
##########
@@ -125,3 +125,68 @@ jobs:
         with:
           name: ${{matrix.image}}-${{matrix.nodejs}}-${{matrix.cpu.platform}}
           path: build/stage/*/*.tar.gz
+
+  windows-napi:
+    name: Build NAPI windows - Node ${{matrix.nodejs}} - ${{matrix.arch}}
+    runs-on: windows-2022
+    timeout-minutes: 3000
+
+    strategy:
+      fail-fast: false
+      matrix:
+        arch:
+          - x64
+          - x86
+        nodejs:
+          - 16
+          - 18
+    steps:
+      - uses: actions/checkout@v3
+      - name: Use Node.js ${{ matrix.nodejs }}
+        uses: actions/setup-node@v3
+        with:
+          node-version: ${{ matrix.nodejs }}
+          architecture: ${{ matrix.arch }}
+          cache: 'npm'
+
+      - name: Cache CPP Client
+        id: cache-dependencies
+        uses: actions/cache@v3
+        with:
+          path: pkg/windows/
+          key: ${{ runner.os }}-${{ matrix.arch }}-${{ hashFiles('tmp-pulsar-client-cpp-version.txt') }}
+
+      - name: Add env vars
+        shell: bash
+        run: |
+          if [ "${{ matrix.arch }}" = "x86" ]; then
+            echo "TARGET=ia32" >> $GITHUB_ENV
+          else
+            echo "TARGET=${{ matrix.arch }}" >> $GITHUB_ENV
+          fi
+
+      - name: Download CPP Client
+        if: steps.cache-dependencies.outputs.cache-hit != 'true'
+        run: |
+          pkg/windows/download-cpp-client.bat ${{ matrix.arch }}
+
+      - name: Build Node binaries lib
+        run: |
+          npm install --ignore-scripts
+          npx node-pre-gyp configure --target_arch=${{ env.TARGET }}
+          npx node-pre-gyp build --target_arch=${{ env.TARGET }}
+
+      - name: Test loading Node binaries lib
+        run: |
+          node pkg/load_test.js
+
+      - name: Package Node binaries lib
+        run: |
+          npx node-pre-gyp package --target_arch=${{ env.TARGET }}
+
+      - name: Upload artifacts
+        uses: actions/upload-artifact@v3
+        if: matrix.nodejs == 16

Review Comment:
   This does not mean that users can only use Node.js 16, because the version of [napi](https://nodejs.org/api/n-api.html#node-api-version-matrix) and the version of Node.js are controlled separately.
   
   The version of NAPI we use is: 4
   
   https://github.com/apache/pulsar-client-node/blob/10ef71a0d235955d91bf763d12513eca51948df0/binding.gyp#L37
   
   Then you can compare. `NAPI 4` applies to Node.js > 10. Refer here: https://nodejs.org/api/n-api.html#node-api-version-matrix
   



-- 
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-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [pulsar-client-node] BewareMyPower merged pull request #238: [feat] Support windows build napi of static link c++ lib.

Posted by GitBox <gi...@apache.org>.
BewareMyPower merged PR #238:
URL: https://github.com/apache/pulsar-client-node/pull/238


-- 
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-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [pulsar-client-node] shibd commented on a diff in pull request #238: [feat] Support windows build napi of static link c++ lib.

Posted by GitBox <gi...@apache.org>.
shibd commented on code in PR #238:
URL: https://github.com/apache/pulsar-client-node/pull/238#discussion_r1009185964


##########
.github/workflows/ci-build-release-napi.yml:
##########
@@ -125,3 +125,68 @@ jobs:
         with:
           name: ${{matrix.image}}-${{matrix.nodejs}}-${{matrix.cpu.platform}}
           path: build/stage/*/*.tar.gz
+
+  windows-napi:
+    name: Build NAPI windows - Node ${{matrix.nodejs}} - ${{matrix.arch}}
+    runs-on: windows-2022
+    timeout-minutes: 3000
+
+    strategy:
+      fail-fast: false
+      matrix:
+        arch:
+          - x64
+          - x86
+        nodejs:
+          - 16
+          - 18
+    steps:
+      - uses: actions/checkout@v3
+      - name: Use Node.js ${{ matrix.nodejs }}
+        uses: actions/setup-node@v3
+        with:
+          node-version: ${{ matrix.nodejs }}
+          architecture: ${{ matrix.arch }}
+          cache: 'npm'
+
+      - name: Cache CPP Client
+        id: cache-dependencies
+        uses: actions/cache@v3
+        with:
+          path: pkg/windows/
+          key: ${{ runner.os }}-${{ matrix.arch }}-${{ hashFiles('tmp-pulsar-client-cpp-version.txt') }}
+
+      - name: Add env vars
+        shell: bash
+        run: |
+          if [ "${{ matrix.arch }}" = "x86" ]; then
+            echo "TARGET=ia32" >> $GITHUB_ENV
+          else
+            echo "TARGET=${{ matrix.arch }}" >> $GITHUB_ENV
+          fi
+
+      - name: Download CPP Client
+        if: steps.cache-dependencies.outputs.cache-hit != 'true'
+        run: |
+          pkg/windows/download-cpp-client.bat ${{ matrix.arch }}
+
+      - name: Build Node binaries lib
+        run: |
+          npm install --ignore-scripts
+          npx node-pre-gyp configure --target_arch=${{ env.TARGET }}
+          npx node-pre-gyp build --target_arch=${{ env.TARGET }}
+
+      - name: Test loading Node binaries lib
+        run: |
+          node pkg/load_test.js
+
+      - name: Package Node binaries lib
+        run: |
+          npx node-pre-gyp package --target_arch=${{ env.TARGET }}
+
+      - name: Upload artifacts
+        uses: actions/upload-artifact@v3
+        if: matrix.nodejs == 16

Review Comment:
   Use 16 and 18 to make sure build correctly in both versions, maybe also need to add 10, 11, 12 ~16 etc.



-- 
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-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [pulsar-client-node] shibd commented on pull request #238: [feat] Support windows build napi of static link c++ lib.

Posted by GitBox <gi...@apache.org>.
shibd commented on PR #238:
URL: https://github.com/apache/pulsar-client-node/pull/238#issuecomment-1296605336

   @merlimat @BewareMyPower PTAL.


-- 
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-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [pulsar-client-node] BewareMyPower commented on a diff in pull request #238: [feat] Support windows build napi of static link c++ lib.

Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on code in PR #238:
URL: https://github.com/apache/pulsar-client-node/pull/238#discussion_r1009109228


##########
.github/workflows/ci-build-release-napi.yml:
##########
@@ -125,3 +125,68 @@ jobs:
         with:
           name: ${{matrix.image}}-${{matrix.nodejs}}-${{matrix.cpu.platform}}
           path: build/stage/*/*.tar.gz
+
+  windows-napi:
+    name: Build NAPI windows - Node ${{matrix.nodejs}} - ${{matrix.arch}}
+    runs-on: windows-2022
+    timeout-minutes: 3000
+
+    strategy:
+      fail-fast: false
+      matrix:
+        arch:
+          - x64
+          - x86
+        nodejs:
+          - 16
+          - 18
+    steps:
+      - uses: actions/checkout@v3
+      - name: Use Node.js ${{ matrix.nodejs }}
+        uses: actions/setup-node@v3
+        with:
+          node-version: ${{ matrix.nodejs }}
+          architecture: ${{ matrix.arch }}
+          cache: 'npm'
+
+      - name: Cache CPP Client
+        id: cache-dependencies
+        uses: actions/cache@v3
+        with:
+          path: pkg/windows/
+          key: ${{ runner.os }}-${{ matrix.arch }}-${{ hashFiles('tmp-pulsar-client-cpp-version.txt') }}
+
+      - name: Add env vars
+        shell: bash
+        run: |
+          if [ "${{ matrix.arch }}" = "x86" ]; then
+            echo "TARGET=ia32" >> $GITHUB_ENV
+          else
+            echo "TARGET=${{ matrix.arch }}" >> $GITHUB_ENV
+          fi
+
+      - name: Download CPP Client
+        if: steps.cache-dependencies.outputs.cache-hit != 'true'
+        run: |
+          pkg/windows/download-cpp-client.bat ${{ matrix.arch }}
+
+      - name: Build Node binaries lib
+        run: |
+          npm install --ignore-scripts
+          npx node-pre-gyp configure --target_arch=${{ env.TARGET }}
+          npx node-pre-gyp build --target_arch=${{ env.TARGET }}
+
+      - name: Test loading Node binaries lib
+        run: |
+          node pkg/load_test.js
+
+      - name: Package Node binaries lib
+        run: |
+          npx node-pre-gyp package --target_arch=${{ env.TARGET }}
+
+      - name: Upload artifacts
+        uses: actions/upload-artifact@v3
+        if: matrix.nodejs == 16

Review Comment:
   Why do you upload artifcats only for Node.js 16?



-- 
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-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [pulsar-client-node] shibd commented on a diff in pull request #238: [feat] Support windows build napi of static link c++ lib.

Posted by GitBox <gi...@apache.org>.
shibd commented on code in PR #238:
URL: https://github.com/apache/pulsar-client-node/pull/238#discussion_r1009144855


##########
.github/workflows/ci-build-release-napi.yml:
##########
@@ -125,3 +125,68 @@ jobs:
         with:
           name: ${{matrix.image}}-${{matrix.nodejs}}-${{matrix.cpu.platform}}
           path: build/stage/*/*.tar.gz
+
+  windows-napi:
+    name: Build NAPI windows - Node ${{matrix.nodejs}} - ${{matrix.arch}}
+    runs-on: windows-2022
+    timeout-minutes: 3000
+
+    strategy:
+      fail-fast: false
+      matrix:
+        arch:
+          - x64
+          - x86
+        nodejs:
+          - 16
+          - 18
+    steps:
+      - uses: actions/checkout@v3
+      - name: Use Node.js ${{ matrix.nodejs }}
+        uses: actions/setup-node@v3
+        with:
+          node-version: ${{ matrix.nodejs }}
+          architecture: ${{ matrix.arch }}
+          cache: 'npm'
+
+      - name: Cache CPP Client
+        id: cache-dependencies
+        uses: actions/cache@v3
+        with:
+          path: pkg/windows/
+          key: ${{ runner.os }}-${{ matrix.arch }}-${{ hashFiles('tmp-pulsar-client-cpp-version.txt') }}
+
+      - name: Add env vars
+        shell: bash
+        run: |
+          if [ "${{ matrix.arch }}" = "x86" ]; then
+            echo "TARGET=ia32" >> $GITHUB_ENV
+          else
+            echo "TARGET=${{ matrix.arch }}" >> $GITHUB_ENV
+          fi
+
+      - name: Download CPP Client
+        if: steps.cache-dependencies.outputs.cache-hit != 'true'
+        run: |
+          pkg/windows/download-cpp-client.bat ${{ matrix.arch }}
+
+      - name: Build Node binaries lib
+        run: |
+          npm install --ignore-scripts
+          npx node-pre-gyp configure --target_arch=${{ env.TARGET }}
+          npx node-pre-gyp build --target_arch=${{ env.TARGET }}
+
+      - name: Test loading Node binaries lib
+        run: |
+          node pkg/load_test.js
+
+      - name: Package Node binaries lib
+        run: |
+          npx node-pre-gyp package --target_arch=${{ env.TARGET }}
+
+      - name: Upload artifacts
+        uses: actions/upload-artifact@v3
+        if: matrix.nodejs == 16

Review Comment:
   Oh, While I was coding. Node.js 16 is the TLS version. Now its TLS changed to 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-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [pulsar-client-node] BewareMyPower commented on a diff in pull request #238: [feat] Support windows build napi of static link c++ lib.

Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on code in PR #238:
URL: https://github.com/apache/pulsar-client-node/pull/238#discussion_r1009170933


##########
.github/workflows/ci-build-release-napi.yml:
##########
@@ -125,3 +125,68 @@ jobs:
         with:
           name: ${{matrix.image}}-${{matrix.nodejs}}-${{matrix.cpu.platform}}
           path: build/stage/*/*.tar.gz
+
+  windows-napi:
+    name: Build NAPI windows - Node ${{matrix.nodejs}} - ${{matrix.arch}}
+    runs-on: windows-2022
+    timeout-minutes: 3000
+
+    strategy:
+      fail-fast: false
+      matrix:
+        arch:
+          - x64
+          - x86
+        nodejs:
+          - 16
+          - 18
+    steps:
+      - uses: actions/checkout@v3
+      - name: Use Node.js ${{ matrix.nodejs }}
+        uses: actions/setup-node@v3
+        with:
+          node-version: ${{ matrix.nodejs }}
+          architecture: ${{ matrix.arch }}
+          cache: 'npm'
+
+      - name: Cache CPP Client
+        id: cache-dependencies
+        uses: actions/cache@v3
+        with:
+          path: pkg/windows/
+          key: ${{ runner.os }}-${{ matrix.arch }}-${{ hashFiles('tmp-pulsar-client-cpp-version.txt') }}
+
+      - name: Add env vars
+        shell: bash
+        run: |
+          if [ "${{ matrix.arch }}" = "x86" ]; then
+            echo "TARGET=ia32" >> $GITHUB_ENV
+          else
+            echo "TARGET=${{ matrix.arch }}" >> $GITHUB_ENV
+          fi
+
+      - name: Download CPP Client
+        if: steps.cache-dependencies.outputs.cache-hit != 'true'
+        run: |
+          pkg/windows/download-cpp-client.bat ${{ matrix.arch }}
+
+      - name: Build Node binaries lib
+        run: |
+          npm install --ignore-scripts
+          npx node-pre-gyp configure --target_arch=${{ env.TARGET }}
+          npx node-pre-gyp build --target_arch=${{ env.TARGET }}
+
+      - name: Test loading Node binaries lib
+        run: |
+          node pkg/load_test.js
+
+      - name: Package Node binaries lib
+        run: |
+          npx node-pre-gyp package --target_arch=${{ env.TARGET }}
+
+      - name: Upload artifacts
+        uses: actions/upload-artifact@v3
+        if: matrix.nodejs == 16

Review Comment:
   I mean, why do you run the build CI with both Node.js 16 and 18 but only 16 is installed. See the matrix you added:
   
   ```yaml
         matrix:
           arch:
             - x64
             - x86
           nodejs:
             - 16
             - 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-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org