-
Notifications
You must be signed in to change notification settings - Fork 20
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
Quote Win32 arguments. #29
Conversation
I think I've fixed it in 2727f07, just a silly oversight on my part. Sorry for the noise, @jandubois. @pjf: The new tests properly pass on Windows. \o/. Not sure if it covers the cases reported in the other tickets, but I think it's on the right track, at least. |
Probably ought to supersede #13. |
2b36a43 fixes
I suspect the issue may be misquoting args to |
Well, I thought 83b7720 was the last failure, and it was the last in
This appears to be due to my changing |
Looks as though redirection (
|
Well, I did all that work to get all the tests passing, bug then noticed this note in the docs:
I've made it so that it now returns 32-bit values from
If there is some other way to make an app exit with a large exit code with multiple-arg calls to run(), I don't know what it is. |
Okay, everything is consistent now, except that one can no longer use the multiple-argument forms or |
Hi, any chance of an release soonish with this fix? I have Windows users still suffering from the lack of shell quoting. :-( Thank you! |
Happy to report that a Windows Sqitch user used this patch and it worked! Any chance of a code review and possibility of a merge and release? Thank you! |
[Bug Fixes] - Fixed a test failure with the MySQL max limit value, mostly exhibited on BSD platforms. - Removed fallback in the PostgreSQL engine on the `$PGUSER` and `$PGPASSWORD` environnement variables, as well as the system username, since libpq does all that automatically, and collects data from other sources that we did not (e.g., the password and connection service files). Thanks to Tom Bloor for the report (issue #410). - Changed dependency validation to prevent an error when a change required from a different project has been reworked. Previously, when a when requiring a change such as `foo:greeble`, Sqitch would raise an error if `foo:greeble` was reworked, suggesting that the dependency be tag-qualified to eliminate ambiguity. Now reworked dependencies may be required without tag-qualification, though tag-qualification should still be specified if functionality as of a particular tag is required. - Added a workaround for the shell quoting issue on Windows. Applies to IPC::System::Simple 1.29 and lower. See [pjf/ipc-system-simple#29](pjf/ipc-system-simple#29) for details (#413). - Fixed an issue with the MariaDB client where a deploy, revert, or verify failure was not properly propagated to Sqitch. Sqitch now passes `--abort-source-on-error` to the Maria `mysql` client to ensure that SQL errors cause the client to abort with an error so that Sqitch can properly handle it. Thanks to @mvgrimes for the original report and, years later, the fix (#209). - Fixed an issue with command argument parsing so that it truly never returns a target without an engine specified, as documented. - Removed documentation for methods that don't exist. - Fixed test failures due to a change in Encode v2.99 that's stricter about `undef` arguments that should be defined. [Improvements] - The Snowflake engine now consults the `connections.warehousename`, `connections.dbname`, and `connections.rolename` variables in the SnowSQL configuration file (`~/.snowsql/config`) before falling back on the hard-coded warehouse name "sqitch" and using the system username as the database name and no default for the role. - Switched to using a constant internally to optimize windows-specific code paths at compile time. - When `deploy` detects undeployed dependencies, it now eliminates duplicates before listing them in the error message. - Now requiring IO::Pager v0.34 or later for its more consistent interface. - Added notes about creating databases to the tutorials. Thanks to Dave Rolsky for the prompt (#315). - Added a status message to tell the user when the registry is being updated, rather than just show each individual update. Thanks to Ben Hutton for the suggestion (#276). - Added support for a `$SQITCH_TARGET` environment variable, which takes precedence over all other target specifications except for command-line options and arguments. Thanks to @mvgrimes for the suggestion (#203). - Fixed target/engine/change argument parsing so it won't automatically fail when `core.engine` isn't set unless no targets are found. This lets engines be determined strictly from command-line arguments -- derived from targets, or just listed on their own -- whether or not `core.engine` is set. This change eliminates the need for the `no_default` parameter to the `parse_args()` method of App::Sqitch Command. It also greatly reduces the need for the core `--engine` option, which was previously required to work around this issue (see below for its removal). - Refactored config handling in tests to use a custom subclass of App::Sqitch::Config instead of various mocks, temporary files, and the like. - Added advice to use the PL/pgSQL `ASSERT()` function for verify scripts to the Postgres tutorial. Thanks to Sergii Tkachenko for the PR (#425). [Target Variables] - The `verify` command now reads `deploy.variables`, and individual `verify.variables override `deploy.variables`, on the assumption that the verify variables in general ought to be the same as the deploy variables. This makes `verify` variable configuration consistent with `revert` variable configuration. - Variables set via the `--set-deploy` option on the `rebase` and `checkout` commands no longer apply to both reverts and deploys, but only deploys. Use the `--set` option to apply a variable to both reverts and deploys. - Added support for core, engine, and target variable configuration. The simplest way to use them is via the `--set` option on the `init`, `engine`, and `target` commands. These commands allow the configuration of database client variables for specific engines and targets, as well as defaults that apply to all change execution commands (`deploy`, `revert`, `verify`, `rebase`, and `checkout`). The commands merge the variables from each level in this priority order: * `--set-deploy` and `--set-revert` options on `rebase` and `checkout` * `--set` option * `target.$target.variables` * `engine.$engine.variables` * `deploy.variables`, `revert.variables`, and `verify.variables` * `core.variables` See `sqitch-configuration` for general documentation of of the hierarchy for merging variables and the documentation for each command for specifics. [Options Unification] - Added the `--chdir`/`--cd`/`-C` option to specify a directory to change to before executing any Sqitch commands. Thanks to Thomas Sibley for the suggestion (#411). - Added the `--no-pager` option to disable the pager (#414). - Changed command-line parsing to allow core and command options to appear anywhere on the line. Previously, core options had to come before the command name, and command options after. No more. The caveat is that command options that take arguments should either appear after the command or use the `--opt=val` syntax instead of `--opt val`, so that Sqitch doesn't think `val` is the command. Even in that case, it will search the rest of the arguments to find a valid command. However, to minimize this challenge, the documentation now suggests and demonstrates putting all options after the command, like so: `sqitch [command] [options]`. - Simplified and clarified the distinction between core and command options by removing all options from the core except those that affect output and runtime context. The core options are: * -C --chdir --cd <dir> Change to directory before performing any actions * --etc-path Print the path to the etc directory and exit * --no-pager Do not pipe output into a pager * --quiet Quiet mode with non-error output suppressed * -V --verbose Increment verbosity * --version Print the version number and exit * --help Show a list of commands and exit * --man Print the introductory documentation and exit - Relatedly, single-letter core options will now always be uppercase, while single-letter command options will be lowercase. As such, `-V` has been added as an alias for `--version`, although `-v` remains for now, undocumented. It may be removed in the future should a compelling use for `-v` in a command be discovered. - All other options have been moved to the commands they affect. Their use should remain mostly unchanged now that command options are parsed from anywhere on the command-line, although we recommend that all options come after commands. The options were moved as follows: * `--registry`, `--client`, `--db-name`, `--db-user`, `--db-host`, and `--db-port` (and their aliases) have been moved to the `checkout`, `deploy`, `log`, `rebase`, `revert`, `status`, `upgrade`, and `verify` commands. * `--plan-file` and `--top-dir` (deprecated; see below) have been moved to the `add`, `bundle`, `checkout`, `deploy`, `rebase`, `revert`, `rework`, `show`, `status`, `tag`, and `verify` commands. They were already supported by the `init`, `engine`, and `target` commands (where `--top-dir` is not deprecated). - Because some command options conflicted with core options, a few options have been removed altogether, including: * The `--verbose` option on the `--engine` and `--target` commands has been removed, but no visible change should be apparent, since those commands now read the core `--verbose` option. * The undocumented `--dir` alias for `--top-dir` has been removed, as it conflicted with the option of the same name but different meaning in the `init`, `engine`, and `target` commands. * The `-d` alias for `--set-deploy` in the `rebase` and `checkout` commands has been changed to `-e` so as not to conflict with the `-d` alias for `--db-name`. * Added tests for all commands to ensure none of their options conflict with core options. Will help prevent conflicts in the future. [Deprecations & Removals] - Deprecated the `--top-dir` option in favor of `--chdir` with a warning except when used for configuration in the `init`, `engine`, and `target` commands. - Removed the core `--deploy-dir`, `--revert-dir`, and `--verify-dir` options, which have been deprecated and triggering warnings since v0.9993 (August 2015). The `--dir` option to the `init`, `engine`, and `target` commands remains the favored interface for specifying script directories. - Removed the deprecated core `--engine` option. The `init` command still supports it, while other commands are able to parse the engine name as an argument — e.g., `sqitch deploy mysql` — or implicitly as part of a target, as in `sqitch revert db:pg:tryme`. When Sqitch is unable to determine the engine for a command, the error message no longer mentions `--engine` and instead suggests specifying the engine via the target. This option never triggered an error, but demonstration of its use has been limited to `init` examples. - Removed support for reading the `core.$engine` configuration, which has been deprecated with warnings in favor of `engine.$engine` since 0.997 (November 2014). The `sqitch engine update-config` action remains available to update old configurations, but may be removed in the future. - Removed the `--deploy`, `--revert`, and `--verify` options on the `add` command, as well as their `--no-*` variants. They have been deprecated with warnings in favor of the `--with` and `--without` options since v0.990 (January 2014). - Removed the `--deploy-template`, `--revert-template`, and `--verify-template` options to the `add` command. They have been deprecated with warnings in favor of the `--use` option since v0.990 (January 2014). - Removed the `add.deploy_template`, `add.revert_template`, and `add.verify_template` configuration settings. They have been deprecated with warnings in favor of the `add.templates` configuration section since v0.990 (January 2014). - Removed the `@FIRST` and `@LAST` symbolic tags, which have been deprecated with warnings in favor of `@ROOT` and `@HEAD`, respectively, since 0.997 (November 2014). - Removed the command-specific options with the string "target" in them, such as `--to-target`, `--upto-target`, which have been deprecated with warnings in in favor of options containing the string "change", such as `--to-change` and `--upto-change`, since v0.997 (November 2014). - Remove the `engine` and `target` command `set-*` actions and their corresponding methods, which have been deprecated in favor of the `alter` action since v0.9993 (August 2015). - Removed the automatic updating of change and tag IDs in the Postgres engine. This functionality was added in v0.940 (December 2012), when Postgres was the only engine, and the SHA-1 hash for change and tag IDs was changed. There were very few deployments at the time, and all should long since have been updated. [API Changes] - Added the URI-overriding parameters `user`, `host`, `port`, and `dbname` to App::Sqitch::Target so that command options can be used to easily set them. - Added support for passing attribute parameters to the `all_targets` group constructor on App::Sqitch::Target, so that command-line options can be used to assign attributes to all targets read from the configuration. - Aded the `target_params` method to App::Sqitch::Command and updated all commands to use it when constructing targets. This allows commands to define options for Target parameters, as required for moving options to commands as described above. - Added the `class_for` method to App::Sqitch::Command so that the new options parser described above can load a command class without instantiating an instance. Useful for searching command-line arguments for a command name. - Added the `create` constructor to App::Sqitch::Command to let Sqitch instantiate an instance of a command once it finds one via `class_for`. Previously, Sqitch used the `load` method, which handled the functionality of both `class_for` and `create`. That method still exists but is used only in tests. - Added the ConnectingCommand role to define database connection options for the commands that need them. - Added the ContextCommand role to define command options for the location of the plan file and top directory. This is also where use of the deprecated form of `--top-dir` triggers a warning. - Removed the `verbosity` attribute from App::Sqitch::Command::engine and App::Sqitch::Command::target, since the `--verbose` option is no longer needed. These commands now rely on the core `--verbose` option. - Removed the copying of core options from the target class and TargetConfigCommand role, since the attributes fetched from there are no longer core options, but provided as attribute parameters to the constructors by commands. - Removed documentation for the optional `config` parameter to the `all_targets` constructor of App::Sqitch::Target, since it was never used by Sqitch. It always fetched the config from the required `sqitch` parameter. Support for the `config` parameter has not been removed, since third-parties might use it. - Removed the `set_*` methods in the `engine` and `target` commands, which have been deprecated in favor of the new `alter` method since v0.9993 (August 2015). - Removed the `old_id` and `old_info` methods from Change and Tag, which date from v0.940 (December 2012), and were provided only to allow existing Postgres databases to be updated from the old to new ID format, now removed. There should be no other use case for these methods.
@theory, would you be able to rebase this pull request on IPC-System-Simple master (9248ce9) and submit as a new pull request? Background to this request: Last year @pjf granted me commit bits to the CPAN and github.com repositories for this distribution. I used that to clear up bugs (including some for which I was the reporter) on *nix platforms. Almost all the remaining issues (https://github.com/pjf/ipc-system-simple/issues) are Windows-related -- but I have neither access to Windows nor sufficient experience to fully address those issues. Recently there has been renewed attention to IPC-System-Simple's problems on Windows in #34, #32, #35 and other issues and pull requests. @dylanstreb; @PhilterPaper. Because of my own limitations (above) I'm not in a position to make a final decision. The substance of your p.r. looks reasonable and I would like, at the very least, to create a branch for it in the main github repository so that other people could try it out. But since the p.r. is a couple of years old it doesn't apply cleanly and, in any event, I'd like to get one single patch that can apply cleanly on top of the HEAD of master. Is that feasible? Thank you very much. |
Add a test demonstrating the failure of the module to handle arguments with spaces on Windows. Then try to fix it by using Win32::ShellQuote. Doesn't quite work, alas.
I had just forgot an extra argument. This gets the tests passing, demonstrating that arguments with spaces are passed properly now.
Use recdirection in a singler string argument to run() to make sure the Win32 quoting does not interfere with this syntax.
We don't want to quote a single-string.
Shell quoting can't be done on `cmd.exe` or its options, and the script must be passed as a single item. So do the quoting manually. This highlights that cmd.exe calls may be broken compared to how they were before. If we wanted to fix that, it would probably require examining the $exe and declining to shell quote args if it's cmd.exe or command.exe. I can't say whether that's desireable. Also, fix run() so that it uses Win32::Process::Create() instead of system and properly checks the exit values. This gets all Win32 tests passing except for RT#48319.
By not doing the quoting manually, but letting capture() do it.
To ensure that shell metacharacters are properlly interpreted. Make sure it's the full path, so it will work even when the path variable is not set. All tests pass on Windows now.
The x variants now always quote it, as do the multi-arg forms. It must be explicitly quoted for the single-argument cals to `run` and `capture`.
Rebased. |
@theory, thanks for the rebase. I have created a branch theory:win32quote and pushed it to the repository. I have also asked people who have filed other Win32-related issues to test it. In my own repository I added an AppVeyor configuration file and activated a build. However, the build failed quickly.
But, needless to say, I don't have access to the cpanm build.log file. Do you have any clue as to which this might happen? Thank you very much. |
Does it process |
The AppVeyor log doesn't provide any information on that. However, the Travis logs for Linux build clearly indicate that Dist::Zilla is being used. So my guess is that you're correct is saying we need Win32::ShellQuote. Do you know how to indicate that in Thank you very much. |
Actually,
But I have no experience with modifying Thank you very much. |
I'm saying that if it's using |
I suggest you replace line 15 with
|
Per recommendation from David Wheeler in pjf#29 (comment)
Unfortunately, that made no difference. From the log file for run 1.0.5 at https://ci.appveyor.com/project/jkeenan/ipc-system-simple:
|
SIGH Sadly, cpanminus does not appear to have a way to send the log output to STDERR or STDOUT. Try something like
Or something like that. I don't know the DOS file globbing syntax, and the backslashes might need to be escaped. Looking at how I dealt with testing Sqitch on Windows, I gave up on using Testing on Windows is a PITA. Worth it, though. |
So maybe
But I'm kind of guessing at the dollar escapes here. Avoiding those might be why I just added a simple |
Using GitHub workflows, it looks like I rely on the system |
I'm trying the KISS approach ... and I got a PASS! See: https://ci.appveyor.com/project/jkeenan/ipc-system-simple/builds/31624928 Thank you very much. |
After junking @theory, thanks very much for your p.r. and subsequent advice. |
Glad to finally see this getting out there. Thank you! |
Turns out 1.26 was released last month without the fix for Win32. Today, however, pjf/ipc-system-simple#29 was merged and a 1.27 trial release made, so it seems likely the fix will be released in 1.27. So update the workaround here to operate on 1.26 and earlier.
Turns out 1.26 was released last month without the fix for Win32. Today, however, pjf/ipc-system-simple#29 was merged and a 1.27 trial release made, so it seems likely the fix will be released in 1.27. So update the workaround here to operate on 1.26 and earlier.
Turns out 1.26 was released in January without the fix for Win32. Recently, however, pjf/ipc-system-simple#29 was merged and 1.28 released, fixing the Win32 issue. So update the workaround here to operate on 1.27 and earlier.
Taking a stab at addressing an issue reported in #9, #14, #22 and #24. I think using Win32::ShellQuote is likely to be the correct approach, but somehow I haven't quite got the syntax down. Perhaps Jan Dubois could point me in the right direction. @jandubois: Any pointers?