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.

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

QOL: Config run should handle shell paths with spaces #989

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

achekery
Copy link

Description

  1. Added terminals.get_short_path_name() utility function that uses ctypes.windll.kernel32.GetShortPathNameW to get short path form for long path. Only applies to Windows-based systems; on Unix this is a pass-thru.
  2. Updated Runner._setup() function to use the new utility function when setting self.opts["shell"] on windows platform.
  3. Added skip_if_posix() decorator function that skips the decorated test when on UNIX.
  4. Updated Runner_.shell test class to check the new behavior.
  5. Updated terminals test class to check the new behavior.

Demo

With invoke main:

  • Both long format and short format work for cmd.exe
  • Although short format works for pwsh.exe, long format does not.
PS > pipx uninstall invoke ; pipx install 'git+https://github.com/pyinvoke/invoke@main' ; invoke demo

** Setup Start: shell_name='cmd.exe', shell_which='C:\\WINDOWS\\system32\\cmd.exe'
** Demo #1-whichformat: with shell_path_='C:\\WINDOWS\\system32\\cmd.exe' run command_='git status --porcelain'
git status --porcelain
 M projects/bce-patch-builder/tasks.py
** Demo Success!: res_=<Result cmd='git status --porcelain' exited=0>
** Demo #1-83format: with shell_path_='C:\\WINDOWS\\system32\\cmd.exe' run command_='git status --porcelain'
git status --porcelain
 M projects/bce-patch-builder/tasks.py
** Demo Success!: res_=<Result cmd='git status --porcelain' exited=0>

==========================================
** Setup Start: shell_name='pwsh.exe', shell_which='C:\\Program Files\\PowerShell\\7\\pwsh.exe'      
** Demo #2-whichformat: with shell_path_='C:\\Program Files\\PowerShell\\7\\pwsh.exe' run command_='git status --porcelain'
git status --porcelain
The argument 'Files\PowerShell\7\pwsh.exe' is not recognized as the name of a script file. Check the spelling of the name, or if a path was included, verify that the path is correct and try again.      

Usage: pwsh[.exe] [-Login] [[-File] <filePath> [args]]
                  [-Command { - | <script-block> [-args <arg-array>]
                                | <string> [<CommandParameters>] } ]
                  [-CommandWithArgs <string> [<CommandParameters>]
                  [-ConfigurationName <string>] [-ConfigurationFile <filePath>]
                  [-CustomPipeName <string>] [-EncodedCommand <Base64EncodedCommand>]
                  [-ExecutionPolicy <ExecutionPolicy>] [-InputFormat {Text | XML}]
                  [-Interactive] [-MTA] [-NoExit] [-NoLogo] [-NonInteractive] [-NoProfile]
                  [-NoProfileLoadTime] [-OutputFormat {Text | XML}]
                  [-SettingsFile <filePath>] [-SSHServerMode] [-STA]
                  [-Version] [-WindowStyle <style>]
                  [-WorkingDirectory <directoryPath>]

       pwsh[.exe] -h | -Help | -? | /?

PowerShell Online Help https://aka.ms/powershell-docs

All parameters are case-insensitive.
** Demo Error: _exc=<UnexpectedExit: cmd='git status --porcelain' exited=64>
** Demo #2-83format: with shell_path_='C:\\PROGRA~1\\POWERS~1\\7\\pwsh.exe' run command_='git status --porcelain'
git status --porcelain
 M projects/bce-patch-builder/tasks.py
** Demo Success!: res_=<Result cmd='git status --porcelain' exited=0>

==========================================

With invoke fork:

  • Both long format and short format work for cmd.exe
  • Both long format and short format work for pwsh.exe
PS > pipx uninstall invoke ; pipx install 'git+https://github.com/achekery/invoke@qol-shellpathswithspaces' ; invoke demo

** Setup Start: shell_name='cmd.exe', shell_which='C:\\WINDOWS\\system32\\cmd.exe'
** Demo #1-whichformat: with shell_path_='C:\\WINDOWS\\system32\\cmd.exe' run command_='git status --porcelain'
git status --porcelain
 M projects/bce-patch-builder/tasks.py
** Demo Success!: res_=<Result cmd='git status --porcelain' exited=0>
** Demo #1-83format: with shell_path_='C:\\WINDOWS\\system32\\cmd.exe' run command_='git status --porcelain'
git status --porcelain
 M projects/bce-patch-builder/tasks.py
** Demo Success!: res_=<Result cmd='git status --porcelain' exited=0>

==========================================
** Setup Start: shell_name='pwsh.exe', shell_which='C:\\Program Files\\PowerShell\\7\\pwsh.exe'      
** Demo #2-whichformat: with shell_path_='C:\\Program Files\\PowerShell\\7\\pwsh.exe' run command_='git status --porcelain'
git status --porcelain
 M projects/bce-patch-builder/tasks.py
** Demo Success!: res_=<Result cmd='git status --porcelain' exited=0>
** Demo #2-83format: with shell_path_='C:\\PROGRA~1\\POWERS~1\\7\\pwsh.exe' run command_='git status --porcelain'
git status --porcelain
 M projects/bce-patch-builder/tasks.py
** Demo Success!: res_=<Result cmd='git status --porcelain' exited=0>

==========================================

@achekery
Copy link
Author

Any suggestions on this change? @bitprophet , @kuwv . Thank you

from invoke.terminals import (
pty_size,
bytes_to_read,
WINDOWS,
Copy link
Contributor

Choose a reason for hiding this comment

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

Objects should precede functions.

.coveragerc Outdated
@@ -3,4 +3,6 @@ branch = True
include =
invoke/*
tests/*
omit = invoke/vendor/*
omit =
invoke/shims.py
Copy link
Contributor

Choose a reason for hiding this comment

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

Coverage is omitted for vendor-ed dependencies because those are separately developed modules. I don't think this should be omitted

@kuwv
Copy link
Contributor

kuwv commented Apr 1, 2024

I don't really know windows. Can you explain what the issue is here?

Thanks

nm, I just read this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants