feat(scripts): add {run_cwd} placeholder for cross-platform scripts - #3815
Conversation
yangfan-yf-yf
left a comment
yangfan-yf-yf
left a comment
There was a problem hiding this comment.
I found a Windows regression in the placeholder substitution.
RE_CWD_PLACEHOLDER.subn(cwd, script) treats cwd as a regular-expression replacement template. A normal Windows working directory such as C:\Users\... contains \U, which re interprets as an invalid replacement escape and raises re.error: bad escape \U at position 2 before the command runs.
I reproduced this with the added coverage on Windows:
python -m pytest tests/cli/test_run.py -k cwd_placeholder -q
6 failed, 74 deselected
This affects both the shell and command modes, including the new spaces/composite cases. Please pass a callable replacement (for example lambda _: cwd) to subn, or otherwise escape the replacement value, so the path is inserted literally.
yangfan-yf-yf
left a comment
yangfan-yf-yf
left a comment
There was a problem hiding this comment.
Thanks for changing the replacement to a callable; that fixes the original \\U replacement-template failure.
There is still a Windows shell-quoting issue in the new for_shell branch. subprocess.list2cmdline() follows the Microsoft C runtime argument-parsing rules, but the result is passed to cmd.exe. For a working directory without whitespace such as C:\\repo&ver, subprocess.list2cmdline([path]) returns C:\\repo&ver without quotes. With cmd.exe /d /c echo C:\\repo&ver, & is then parsed as a command separator (the output includes both the path and the Windows version) rather than as part of the path.
Could the shell-specific interpolation escape or quote cmd.exe metacharacters even when the path has no spaces, and add a regression case for such a path? The existing spaced-path test does not cover this branch.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3815 +/- ##
=======================================
Coverage 88.32% 88.33%
=======================================
Files 121 121
Lines 13213 13215 +2
Branches 2244 2246 +2
=======================================
+ Hits 11671 11673 +2
Misses 971 971
Partials 571 571
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@pctablet505 all other placeholders are lowercased, can you use lowercase, too? |
frostming
changed the title
Adds a {PDM_RUN_CWD} placeholder that expands to the absolute path of the directory from which pdm run was invoked, matching the PDM_RUN_CWD environment variable. This lets shell and cmd scripts reference the cwd in a cross-platform way.
Fixes pdm-project#3734
- Add news/3734.feature.md describing the new placeholder.
- Close backtick in _interpolate_cwd docstring.
- Update interpolate docstring to mention {pdm} and {PDM_RUN_CWD}.
- Make _interpolate_cwd return whether substitution occurred so cmd-list
parts are properly shlex.split after quoting paths containing spaces.
- Add tests for shell, cmd, and composite scripts with spaces in cwd.
…dows Shell scripts are handed straight to the OS shell, which on Windows is cmd.exe. cmd.exe does not treat single quotes as quoting syntax, so a working directory containing spaces (e.g. C:\Users\Jane Doe\Project) was passed to the invoked program garbled, defeating the whole point of the placeholder on the platform it exists to help. Quote the interpolated cwd per-consumer: for shell scripts on Windows use subprocess.list2cmdline (cmd.exe-safe double quotes); keep POSIX shlex.quote everywhere else, since cmd and composite parts are always re-parsed with shlex.split regardless of platform. Fixes pdm-project#3734
…e errors Windows paths (e.g. C:\Users\...) contain sequences like \U that re.subn() interprets as invalid replacement-string escapes when passed as a literal string, raising re.error instead of substituting the placeholder.
subprocess.list2cmdline implements the MSVCRT argv-quoting convention
used by CreateProcess, not cmd.exe's own command-line grammar. Once the
quoted path reaches cmd.exe (which is what shell scripts run under on
Windows), that mismatch causes two separate problems:
- cmd.exe operators such as & are still parsed as command separators
inside list2cmdline's double quotes, e.g. a cwd of C:\repo&ver turns
`echo {PDM_RUN_CWD}` into two commands instead of one.
- Windows CI is currently red because cmd.exe builtins like echo don't
strip surrounding quotes the way an argv-consuming program would, so
even the existing spaces-in-path case prints the quotes literally.
Escape cmd.exe's own operator metacharacters (^ & | < > ( )) with
carets instead of quoting the whole path. This is the escaping cmd.exe
itself defines for its command-line parser, it composes correctly
regardless of whether the path also contains whitespace, and it leaves
plain paths completely unchanged since echo does not tokenize on
spaces.
Fixes pdm-project#3734
All other script placeholders (`{args}`, `{pdm}`) are lowercase; the
new one was uppercase, inconsistent with that convention. Renamed the
placeholder token only -- the PDM_RUN_CWD environment variable itself
stays uppercase, matching every other env var this project exposes
(PDM_PROJECT_ROOT, PDM_NO_CACHE, etc).
d69d087 to
919362e
Compare
…s scripts Signed-off-by: Frost Ming <me@frostming.com>
frostming
changed the title
|
FYI @pctablet505 I've jumped in and changed to the name to |
…ces_in_path to remove extra argument and assert correct output Signed-off-by: Frost Ming <me@frostming.com>
3 participants
Fixes #3734
Adds a
{PDM_RUN_CWD}placeholder for PDM scripts so callers can keep PDM-specific paths out of tools that cannot read environment variables. Works incmd,shell, andcompositescripts; paths with spaces are handled viashlex.quote. Includes tests and docs.