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.

Sign up for GitHub

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

Jump to bottom

test: Run tests on Node.js v22 #244

Draft
wants to merge 20 commits into
base: main
Choose a base branch
from
Draft

test: Run tests on Node.js v22 #244

wants to merge 20 commits into from
+19 −8

Conversation

cclauss
Copy link
Contributor

@cclauss cclauss commented Apr 25, 2024

@targos
Copy link
Member

targos commented Apr 25, 2024 •
edited by cclauss

Windows CI is broken by:

@targos
Copy link
Member

targos commented Apr 25, 2024 •
edited by cclauss

For actions/setup-python errors, it seems to be:

@cclauss cclauss marked this pull request as draft April 25, 2024 13:15
@cclauss
Copy link
Contributor Author

cclauss commented Apr 25, 2024 •
edited

Python issues were fixed by explicitly selecting macos-13 and macos-14 (X64 Intel vs. ARM64 Apple Silicon).

@cclauss
Copy link
Contributor Author

cclauss commented Apr 25, 2024 •
edited

Win: https://community.chocolatey.org/packages/nodejs

Mac: Homebrew/homebrew-core#169999

@cclauss
Copy link
Contributor Author

cclauss commented May 1, 2024 •
edited

https://github.com/npm/cli/releases/tag/v10.7.0 in GitHub Actions:

      - uses: actions/setup-node@v4
        with:
          node-version: 22.x

      - if: runner.os == 'Windows'
        shell: bash  # Not pwsh!!!
        run: |
          npm --version  # 10.5.1
          npm install -g npm  # >= https://github.com/npm/cli/releases/tag/v10.7.0
          npm --version  # 10.7.0

Now the tests fail further down in Python code…

File "C:\hostedtoolcache\windows\Python\3.12.3\x64\Lib\encodings\cp1252.py", line 19, in encode
    return codecs.charmap_encode(input,self.errors,encoding_table)[0]
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
UnicodeEncodeError: 'charmap' codec can't encode character '\u012b' in position 3: character maps to <undefined>

@cclauss cclauss requested a review from legendecas May 1, 2024 05:12
@cclauss cclauss added the help wanted Extra attention is needed label May 1, 2024
@legendecas
Copy link
Member

node-gyp integration failures are related to: nodejs/nan#968

@legendecas
Copy link
Member

The codecs.charmap_encode error outputs are noises as it is used to skip tests: https://github.com/nodejs/node-gyp/blob/main/test/test-addon.js#L59

@legendecas legendecas mentioned this pull request May 2, 2024
Merged
@cclauss
Copy link
Contributor Author

cclauss commented May 2, 2024

https://github.com/actions/node-versions/releases/tag/22.1.0-8926142033

@cclauss cclauss closed this May 3, 2024
@cclauss cclauss deleted the Node.js_v22 branch May 3, 2024 20:04
nodejs-github-bot pushed a commit to nodejs/node that referenced this pull request May 4, 2024
@legendecas
build: define NOMINMAX in common.gypi
8b2011a 
V8 and Node.js had defined `NOMINMAX` on Windows for a long time.  In
recent changes, V8 added `std::numeric_limits::min` usages in its
header files which caused addons without `NOMINMAX` defines failed
to compile.

Define `NOMINMAX` in common.gypi so that addons can be compiled with
the latest V8 header files.

PR-URL: #52794
Fixes: nodejs/nan#968
Refs: nodejs/gyp-next#244
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
@cclauss cclauss restored the Node.js_v22 branch May 5, 2024 12:45
@cclauss cclauss reopened this May 5, 2024
Ch3nYuY pushed a commit to Ch3nYuY/node that referenced this pull request May 8, 2024
@legendecas @Ch3nYuY
build: define NOMINMAX in common.gypi
b43011f 
V8 and Node.js had defined `NOMINMAX` on Windows for a long time.  In
recent changes, V8 added `std::numeric_limits::min` usages in its
header files which caused addons without `NOMINMAX` defines failed
to compile.

Define `NOMINMAX` in common.gypi so that addons can be compiled with
the latest V8 header files.

PR-URL: nodejs#52794
Fixes: nodejs/nan#968
Refs: nodejs/gyp-next#244
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
targos pushed a commit to nodejs/node that referenced this pull request May 8, 2024
@legendecas @targos
build: define NOMINMAX in common.gypi
e1bd53c 
V8 and Node.js had defined `NOMINMAX` on Windows for a long time.  In
recent changes, V8 added `std::numeric_limits::min` usages in its
header files which caused addons without `NOMINMAX` defines failed
to compile.

Define `NOMINMAX` in common.gypi so that addons can be compiled with
the latest V8 header files.

PR-URL: #52794
Fixes: nodejs/nan#968
Refs: nodejs/gyp-next#244
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
@cclauss
Copy link
Contributor Author

cclauss commented May 8, 2024

@legendecas @StefanStojanovic Any idea how we can fix the UnicodeEncodeError so we can land this in

@cclauss cclauss mentioned this pull request May 8, 2024
Merged
@legendecas
Copy link
Member

@cclauss the unicode error is unrelevant, see #244 (comment). The real error is caused by nodejs/node#52794, which should be released in the next week's new v22 version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@targos targos Awaiting requested review from targos

@lukekarrys lukekarrys Awaiting requested review from lukekarrys

@legendecas legendecas Awaiting requested review from legendecas

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
help wanted Extra attention is needed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@cclauss @targos @legendecas