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

Merged
merged 1 commit into from
May 26, 2024
Merged

test: Run tests on Node.js v22 #244

merged 1 commit into from
May 26, 2024

Conversation

cclauss
Copy link
Contributor

@cclauss cclauss commented Apr 25, 2024

@cclauss cclauss requested review from targos and lukekarrys April 25, 2024 11:40
@targos
Copy link
Member

targos commented Apr 25, 2024 •
edited by cclauss
Loading

Windows CI is broken by:

@targos
Copy link
Member

targos commented Apr 25, 2024 •
edited by cclauss
Loading

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
Loading

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
Loading

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

Mac: Homebrew/homebrew-core#169999

@cclauss
Copy link
Contributor Author

cclauss commented May 1, 2024 •
edited
Loading

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 <[email protected]>
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
@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
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 <[email protected]>
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
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 <[email protected]>
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
@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.

@StefanStojanovic
Copy link
Contributor

@cclauss Node.js v22.2 was released, would it make sense to resolve conflicts and try this now without the Windows exclude?

@cclauss cclauss marked this pull request as ready for review May 25, 2024 16:49
StefanStojanovic
StefanStojanovic approved these changes May 26, 2024
View reviewed changes
Copy link
Contributor

@StefanStojanovic StefanStojanovic left a comment

Choose a reason for hiding this comment

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

LGTM since the checks are green.

@gengjiawen gengjiawen merged commit 9ea7409 into nodejs:main May 26, 2024
35 checks passed
@cclauss cclauss deleted the Node.js_v22 branch May 26, 2024 12:29
sophoniie pushed a commit to sophoniie/node that referenced this pull request Jun 20, 2024
@legendecas @sophoniie
build: define NOMINMAX in common.gypi
e0a59ce 
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 <[email protected]>
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
bmeck pushed a commit to bmeck/node that referenced this pull request Jun 22, 2024
@legendecas @bmeck
build: define NOMINMAX in common.gypi
5e11ec1 
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 <[email protected]>
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@StefanStojanovic StefanStojanovic StefanStojanovic approved these changes

@gengjiawen gengjiawen gengjiawen approved these changes

@targos targos Awaiting requested review from targos

@lukekarrys lukekarrys Awaiting requested review from lukekarrys

@legendecas legendecas Awaiting requested review from legendecas

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.
5 participants
@cclauss @targos @legendecas @StefanStojanovic @gengjiawen