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

fix: add windowsVerbatimArguments to not add ' in the argument for Windows environment #753

Merged
merged 5 commits into from
Feb 27, 2022

Conversation

KazuCocoa
Copy link
Member

@KazuCocoa KazuCocoa commented Feb 19, 2022

Fixes #752

@KazuCocoa KazuCocoa marked this pull request as draft February 19, 2022 19:10
@@ -74,7 +74,7 @@ class ServerBuilder {

getCommand () {
const cmd = system.isWindows() ? 'gradlew.bat' : './gradlew';
const buildProperty = (key, value) => value ? `-P${key}=${value}` : null;
const buildProperty = (key, value) => value ? `-P${key}="${value}"` : null;
Copy link
Contributor

Choose a reason for hiding this comment

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

I would rather check explicitly if the corresponding value already contains quotes and throw an error.

Copy link
Member Author

Choose a reason for hiding this comment

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

It seems like the root cause was -PappiumTargetPackage="io.appium.android.apis" etc was wrapped by ' in the command. So, with ", the command will be like:

[Espresso] Unable to build Espresso server - Process ended with exitcode 1 (cmd: './gradlew '-PappiumTargetPackage="io.appium.android.apis"' app\:assembleAndroidTest')

But expected one should be ./gradlew -PappiumTargetPackage="io.appium.android.apis" app\:assembleAndroidTest

Copy link
Member Author

@KazuCocoa KazuCocoa Feb 25, 2022

Choose a reason for hiding this comment

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

okay, i figured out.

The command will be ./gradlew '-PappiumKeyPassword=START&END'.

Then, macOS and Linux have no issue. They handle "START&END" as a string.
But on Windows, ./gradlew '-PappiumKeyPassword=START&END' will be ./gradlew -PappiumKeyPassword=START and END in the command prompt. in the case, the argument should be ./gradlew -PappiumKeyPassword="START&END". In powershell, it should be ./gradlew -PappiumKeyPassword="START\&END" (escape)

Copy link
Contributor

Choose a reason for hiding this comment

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

@KazuCocoa The gradle documentation says

Options that accept values can be specified with or without = between the option and argument; however, use of = is recommended.

So we could maybe try to make the value to a separate arg instead of quoting the whole expression

Copy link
Member Author

Choose a reason for hiding this comment

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

hm, I tried like the below command, but then, the separated arg was handled as a task.

% ./gradlew -PappiumKeyPassword "START&END" app:assembleAndroidTest 

FAILURE: Build failed with an exception.

* What went wrong:
Task 'START&END' not found in root project 'espresso-server'.

* Try:
Run gradlew tasks to get a list of available tasks. Run with --stacktrace option to get the stack trace. Run with --info or --debug option to get more log output. Run with --scan to get full insights.

* Get more help at https://help.gradle.org

I've combined args as a command like:

    const gradlebuild = new SubProcess(cmd + ' ' + args.join(' '),[], {
      cwd: this.serverPath,
      stdio: ['ignore', 'pipe', 'pipe'],
    });

, then, the generated command was ./gradlew -PappiumTargetPackage="io.appium.android.apis" app:assembleAndroidTest ENOENT. So the ENOENT prevent the command. Maybe i need to see SubProcess

@KazuCocoa
Copy link
Member Author

ok... so, for now, this PR can focus on wrapping arguments with " only. #752 (comment)

@@ -163,6 +163,7 @@ class ServerBuilder {
const gradlebuild = new SubProcess(cmd, args, {
cwd: this.serverPath,
stdio: ['ignore', 'pipe', 'pipe'],
windowsVerbatimArguments: true
Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

#752 (comment)
Ok, so, with this one, at least ' in Windows env will gone. Then, they can provide "xxxx" as the capability to avoid
the Windows's escape error....

Copy link
Member Author

Choose a reason for hiding this comment

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

So, with this PR and "START&END" as the capability, the

END is not recognized as an internal or external command,

error should fix.

Copy link
Member Author

@KazuCocoa KazuCocoa Feb 27, 2022

Choose a reason for hiding this comment

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

I'd like to add " automatically on Windows case after this merge and ask the reporter to try out this (if needed)

@KazuCocoa KazuCocoa changed the title fix: add double quote for gradle build config fix: add windowsVerbatimArguments to not add ' in the argument for Windows environment Feb 27, 2022
@KazuCocoa KazuCocoa marked this pull request as ready for review February 27, 2022 03:22
@KazuCocoa KazuCocoa merged commit 69f344c into appium:master Feb 27, 2022
@KazuCocoa KazuCocoa deleted the add-quote branch February 27, 2022 10:06
github-actions bot pushed a commit that referenced this pull request Feb 27, 2022
## [2.1.1](v2.1.0...v2.1.1) (2022-02-27)

### Bug Fixes

* add windowsVerbatimArguments to not add `'` in the argument for Windows environment ([#753](#753)) ([69f344c](69f344c))
@github-actions
Copy link

🎉 This PR is included in version 2.1.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

Windows: Escape ampersand in keystore password
2 participants