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

Configure warning with Python 3.12 #50671

Closed
targos opened this issue Nov 11, 2023 · 9 comments · Fixed by #50695
Closed

Configure warning with Python 3.12 #50671

targos opened this issue Nov 11, 2023 · 9 comments · Fixed by #50695
Labels
python PRs and issues that require attention from people who are familiar with Python.

Comments

@targos
Copy link
Member

targos commented Nov 11, 2023 

$ ./configure --ninja
Node.js configure: Found Python 3.12.0...
<string>:29: SyntaxWarning: invalid escape sequence '\P'
INFO: configure completed successfully

@nodejs/python
@targos targos added the python PRs and issues that require attention from people who are familiar with Python. label Nov 11, 2023
@targos
Copy link
Member Author

targos commented Nov 11, 2023

There are also warnings in V8 during build (this should be fixed upstream):

/Users/mzasso/git/nodejs/node/tools/v8_gypfiles/../../deps/v8/tools/gen-postmortem-metadata.py:747: SyntaxWarning: invalid escape sequence '\s'
  types[re.sub('\s*=.*', '', entry).lstrip()] = True;
/Users/mzasso/git/nodejs/node/tools/v8_gypfiles/../../deps/v8/tools/gen-postmortem-metadata.py:760: SyntaxWarning: invalid escape sequence '\s'
  args = re.split('\s*,\s*', rest);
/Users/mzasso/git/nodejs/node/tools/v8_gypfiles/../../deps/v8/tools/gen-postmortem-metadata.py:843: SyntaxWarning: invalid escape sequence '\s'
  args = re.split('\s*,\s*', rest);
/Users/mzasso/git/nodejs/node/tools/v8_gypfiles/../../deps/v8/tools/gen-postmortem-metadata.py:942: SyntaxWarning: invalid escape sequence '\s'
  ws = re.compile('\s+')

@cclauss
Copy link
Contributor

cclauss commented Nov 12, 2023 •
edited
Loading

This is a warning about Python raw strings...

'\P' --> r'\P'
'\s' --> r'\s'

I am high in the mountains and running out of batteries so I cannot create a pull request.

These are Warnings, not Errors but should be fixed.

@targos
Copy link
Member Author

targos commented Nov 12, 2023

I can't find where the warning is coming from. There is no \P sequence in any .py file in the project.

targos added a commit to targos/node that referenced this issue Nov 13, 2023
@targos
deps: escape Python strings correctly
03b33ce 
Fixes: nodejs#50671
@targos targos mentioned this issue Nov 13, 2023
Merged
targos added a commit to targos/node that referenced this issue Nov 13, 2023
@targos
deps: escape Python strings correctly
0a3224b 
Fixes: nodejs#50671
@targos
Copy link
Member Author

targos commented Nov 13, 2023

Our problem was in a gyp file: #50695

V8 CL: https://chromium-review.googlesource.com/c/v8/v8/+/5018593

@MrJithil
Copy link
Member

The warning <string>:29: SyntaxWarning: invalid escape sequence '\P' is caused by the below line.

node/deps/openssl/openssl_common.gypi

Line 29 in 6322d4f

'OPENSSLDIR="C:\\\Program\ Files\\\Common\ Files\\\SSL"',

@targos
Copy link
Member Author

targos commented Nov 13, 2023

@MrJithil Yes, that's what I fixed in #50695

@MrJithil
Copy link
Member

MrJithil commented Nov 13, 2023 •
edited
Loading

We could either ignore this warning using warnings.filterwarnings

Or we need to modify the file opening logics to read contents as raw text or proper escapes.

Since this is coming from the dependancy, I prefer to ignore this specific warning. Because, I am confused on escaping every deps file is good or not.

To ignore this warning
warnings.filterwarnings("ignore", r"invalid escape sequence", category=SyntaxWarning) at the top of the configure.py

@MrJithil
Copy link
Member

MrJithil commented Nov 13, 2023 •
edited
Loading

I drafted this comment early today, and submitted now. Thanks for that.

@cclauss
Copy link
Contributor

cclauss commented Nov 13, 2023

The warning is useful and it should not be ignored. It will become a SyntaxError in the future so let's solve instead of hide.
https://docs.python.org/3.12/whatsnew/3.12.html#other-language-changes

nodejs-github-bot pushed a commit that referenced this issue Nov 14, 2023
@targos
deps: escape Python strings correctly
36ecf8c 
Fixes: #50671
PR-URL: #50695
Reviewed-By: Christian Clauss <cclauss@me.com>
Reviewed-By: Jithil P Ponnan <jithil@outlook.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
targos added a commit that referenced this issue Nov 23, 2023
@targos
deps: escape Python strings correctly
5d28f8d 
Fixes: #50671
PR-URL: #50695
Reviewed-By: Christian Clauss <cclauss@me.com>
Reviewed-By: Jithil P Ponnan <jithil@outlook.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
martenrichter pushed a commit to martenrichter/node that referenced this issue Nov 26, 2023
@targos @martenrichter
deps: escape Python strings correctly
032d766 
Fixes: nodejs#50671
PR-URL: nodejs#50695
Reviewed-By: Christian Clauss <cclauss@me.com>
Reviewed-By: Jithil P Ponnan <jithil@outlook.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
UlisesGascon pushed a commit that referenced this issue Dec 11, 2023
@targos @UlisesGascon
deps: escape Python strings correctly
b7218b4 
Fixes: #50671
PR-URL: #50695
Reviewed-By: Christian Clauss <cclauss@me.com>
Reviewed-By: Jithil P Ponnan <jithil@outlook.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
UlisesGascon pushed a commit that referenced this issue Dec 19, 2023
@targos @UlisesGascon
deps: escape Python strings correctly
3d82d38 
Fixes: #50671
PR-URL: #50695
Reviewed-By: Christian Clauss <cclauss@me.com>
Reviewed-By: Jithil P Ponnan <jithil@outlook.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
richardlau pushed a commit that referenced this issue Mar 19, 2024
@targos @richardlau
deps: escape Python strings correctly
4a6f83e 
Fixes: #50671
PR-URL: #50695
Reviewed-By: Christian Clauss <cclauss@me.com>
Reviewed-By: Jithil P Ponnan <jithil@outlook.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
python PRs and issues that require attention from people who are familiar with Python.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

deps: escape Python strings correctly targos/node
3 participants
@targos @cclauss @MrJithil