Skip to content

strip double quote in path #3

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

strip double quote in path #3

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions gyp/pylib/gyp/generator/make.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2484,6 +2484,8 @@ def Pchify(self, path, lang):
def Absolutify(self, path):
"""Convert a subdirectory-relative path into a base-relative path.
Skips over paths that contain variables."""
"""In MINGW node -p return a path with double quote"""
path = path.strip('"')
Comment on lines +2487 to +2488
Copy link

@gemini-code-assist gemini-code-assist bot Sep 27, 2025

Choose a reason for hiding this comment

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

medium

This string literal acts as a comment, but it's better to use a standard hash (#) comment for clarity and to follow Python conventions. String literals that are not docstrings or assigned to variables are evaluated and then discarded, which can be slightly less efficient and might be confusing to other developers. I've also slightly rephrased the comment for clarity.

Suggested change
"""In MINGW node -p return a path with double quote"""
path = path.strip('"')
# In MINGW, `node -p` can return a path enclosed in double quotes.
path = path.strip('"')

Copy link

@cclauss cclauss Sep 27, 2025

Choose a reason for hiding this comment

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

Given that this is a MINGW-only issue, would there be a way to rewrite as:

if on_mingw:
    path = path.strip('"')

Copy link
Author

@lsq lsq Sep 27, 2025

Choose a reason for hiding this comment

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

Kreijstal‘s fork be used for msys2 user，any plan merge to officail repo need to check MINGW platform.

Copy link
Owner

@Kreijstal Kreijstal Sep 27, 2025

Choose a reason for hiding this comment

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

I mean yes but this branch is also a PR itself
nodejs#3166
so I would like to be friendly with node-gyp upstream eventually they should merge it of course...

if "$(" in path:
# Don't call normpath in this case, as it might collapse the
# path too aggressively if it features '..'. However it's still
Expand Down
Loading