Reworked run Command Parsing
#50559
Merged
Reworked run Command Parsing
#50559
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This commit removes the manual command parsing in favor of letting Docker handle it the way that it normally would. It adds double dash support to pass options to the container and removes the second shell that was further handling any quotes and shell tokens.
|
Size Change: -290 B (0%) Total Size: 1.4 MB
ℹ️ View Unchanged
|
It looks like we were quoting it, so, fixed!
noahtallen
reviewed
May 17, 2023
There was a problem hiding this comment.
This mostly makes sense tome. I do wish that the quotation syntax that worked before (npx wp-env run cli "echo hello world") still worked, but that seems hard to support.
The main reason is that we spent a while telling folks to use quotation marks to make it work, so this could be a confusing change.
Something which might help is a bonus help message if we notice the command begins with a quotation mark? Though at the same time, I'm not sure exactly how that gets interpreted. It seems like if you pass cli "echo hello world", it thinks the actual binary is "echo hello world", including spaces? In that case, maybe we could provide some help if the binary includes spaces?
| @@ -312,15 +312,15 @@ | |||
| "test:performance": "wp-scripts test-e2e --config packages/e2e-tests/jest.performance.config.js", | |||
| "test:performance:debug": "wp-scripts --inspect-brk test-e2e --runInBand --no-cache --verbose --config packages/e2e-tests/jest.performance.config.js --puppeteer-devtools", | |||
| "test:php": "npm-run-all lint:php test:unit:php", | |||
| "test:php:watch": "wp-env run composer run-script test:watch", | |||
| "test:php:watch": "wp-env run --env-cwd='wp-content/plugins/gutenberg' tests-cli composer run-script test:watch", | |||
| "test:unit": "wp-scripts test-unit-js --config test/unit/jest.config.js", | ||
| "test:unit:date": "bash ./bin/unit-test-date.sh", | ||
| "test:unit:debug": "wp-scripts --inspect-brk test-unit-js --runInBand --no-cache --verbose --config test/unit/jest.config.js ", | ||
| "test:unit:profile": "wp-scripts --cpu-prof test-unit-js --runInBand --no-cache --verbose --config test/unit/jest.config.js ", | ||
| "pretest:unit:php": "wp-env start", | ||
| "test:unit:php": "wp-env run --env-cwd=\"wp-content/plugins/gutenberg\" tests-wordpress vendor/bin/phpunit -c phpunit.xml.dist --verbose", | ||
| "test:unit:php": "wp-env run --env-cwd='wp-content/plugins/gutenberg' tests-wordpress vendor/bin/phpunit -c phpunit.xml.dist --verbose", |
There was a problem hiding this comment.
I noticed the explicit -c phpunit.xml.dist isn't required, but i suppose it's good to be explicit
|
|
||
| ### Enhancement | ||
|
|
||
| - Support using double dashes in `wp-env run ...` to pass arguments that would otherwise be consumed by `wp-env`. For example, while normally `--help` would provide the `wp-env` help text, if you use `npx wp-env run cli php -- --help` you will see the PHP help text. |
There was a problem hiding this comment.
The thing that gets me about this change is that I have always hated the -- --help syntax from npm :/
I personally prefer npx wp-env run cli 'php --help' over npx wp-env run cli php -- --help
There was a problem hiding this comment.
Unfortunately, this is just a consequence of using yargs. It eats any options that aren't defined unless you use a double-dash to tell it to stop parsing everything after it.
Technically the only format that was supported before was quoting the entire command, otherwise, it would break. We could detect that case and provide help text? |
Yeah -- I guess this depends on the command in question. For example , but I think detecting quotes around the entire command is a good idea to reduce friction. we just need to know if it starts with a quote |
|
Flaky tests detected in 0f05032. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5050041512
|
ObliviousHarmony
force-pushed
the
fix/wp-env-run-quoting
branch
from
c319f33 to
c3aba22
Compare
|
@noahtallen I added detection for the command, but, I realized something just now. Technically you can use quotes to call commands that have spaces in the path, right? Maybe we should just wrap the Docker error if it isn't informative enough and give a better explanation? Personally, I kind of feel like the error it gives now is relatively clear, but, I'd be curious to know what you think. I'll leave it as-is for now, but, we should address before shipping. |
This reverts commit c3aba22.
|
I went ahead and reverted the error messaging @noahtallen. Right now you get:
This is the same error you get if you run something that outright doesn't exist like I took a look at giving a better error message for missing commands but since we're just inheriting the I/O streams from the parent process we would need to make some potentially touchy changes to manually pipe the output and validate it. We can go down this route if we think it's worthwhile. I might be suffering from the burden of knowledge here, but, I think that error seems clear, albeit less pretty than we got out of |
|
That's a fair point. I would prefer a better error message, as the OCI runtime stuff is just extra noise. But spaces in the path is a good use case |
noahtallen
approved these changes
May 23, 2023
|
Thanks @noahtallen! |
cbravobernal
added
the
[Type] Code Quality
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
This reworks the handling of
wp-env run's command parsing in order to more consistently handle quotes and shell tokens.Why?
In reworking our E2E setup script I wanted to remove the quotes that #41179 made unnecessary. When I attempted to do so, however, I found that quotes and environment variables exhibited some very strange behavior. On further examination I found that this was because we were calling
docker-composewithshell: trueset. This was causing the command to be processed by two shells before being passed to Docker.How?
Originally this pull request only removed the
shell: trueoption, but when I did that, it broke everything else. The problem I found was that we were heavily relying on the second shell to process the input before runningdocker-compose, and without that shell, we needed to handle that ourselves. The solution here was to just pass the arguments verbatim to Docker, and as a bonus, this madewp-enventirely consistent withdocker-compose exec.The caveat, however, was that quoted string were now being passed as single arguments (since they are!) instead of separate ones. Rather than splitting the string and making an assumption about what the user was trying to do, I opted for having this as a breaking change. Another problem though: we were relying on quoted strings to support passing options that
wp-envwould otherwise have consumed (like--help). The solution I took was to add double dash support since that's pretty standard and already supported out of the box byyargs.The end result is a much more explicit
wp-env runthat does not perform any additional processing on your command. It takes the input it is given (after your local shell processes it) and passes it to Docker verbatim. This resolves any oddities around quoting and a bug wherein you can't pass environment variable tokens to Docker since the local shell will always parse them.Testing Instructions
npx wp-env run cli bash.npx wp-env run cli php --help.npx wp-env run cli php -- --help.npx wp-env run cli "php".npx wp-env run cli "php" \"--help\". Notice that it gives an error, but also, that it is passing the quotes as you would expect it to.