wp-env: Make env-cwd option work on Windows #56265
Merged
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
t-hamano
added
[Type] Bug
|
Warning: Type of PR label mismatch To merge this PR, it requires exactly 1 label indicating the type of PR. Other labels are optional and not being checked here.
Read more about Type labels in Gutenberg. Don't worry if you don't have the required permissions to add labels; the PR reviewer should be able to help with the task. |
1 similar comment
|
Warning: Type of PR label mismatch To merge this PR, it requires exactly 1 label indicating the type of PR. Other labels are optional and not being checked here.
Read more about Type labels in Gutenberg. Don't worry if you don't have the required permissions to add labels; the PR reviewer should be able to help with the task. |
t-hamano
changed the title
--env-cwd option not working on Windows OS
t-hamano
force-pushed
the
fix/scripts-env-cwd-windows
branch
from
ef777c3 to
41756b1
Compare
|
Size Change: +805 B (0%) Total Size: 1.72 MB
ℹ️ View Unchanged
|
It's because Windows treats single quotes as a literal character instead of a special one. This is normally fine and interpreted by Linux correctly but here it's a problem because |
Does this mean, for example, removing single quotes on the env package side as shown below? This approach also seems to work on Windows. diff --git a/packages/env/lib/cli.js b/packages/env/lib/cli.js
index 1788315b60..df8d1830e7 100644
--- a/packages/env/lib/cli.js
+++ b/packages/env/lib/cli.js
@@ -205,6 +205,7 @@ module.exports = function cli() {
default: '.',
describe:
"The command's working directory inside of the container. Paths without a leading slash are relative to the WordPress root.",
+ coerce: ( value ) => value.replace( /'/g, '' ),
} );
args.positional( 'container', {
type: 'string', |
Looks great @t-hamano. One consideration is that this should only remove single quotes from the beginning and end of the path. Another thought along these lines too is whether or not paths with spaces in them work too. |
t-hamano
changed the title
I changed it to remove the leading and trailing single quotes and spaces. I have confirmed that most commands work on Windows and Linux. {
"scripts": {
"no-quote": "wp-env run tests-wordpress --env-cwd=wp-content/plugins/gutenberg pwd",
"has-quote": "wp-env run tests-wordpress --env-cwd='wp-content/plugins/gutenberg' pwd",
"has-escaped-quote": "wp-env run tests-wordpress --env-cwd=\"wp-content/plugins/gutenberg\" pwd",
"has-quote-inside-with-escaped-quote": "wp-env run tests-wordpress --env-cwd=\"wp-content/plugin's/gutenberg\" pwd"
}
}There are other scenarios where the results differ depending on the OS. The following command works on Windows, but has syntax errors on Linux. {
"scripts": {
"has-quote-inside": "wp-env run tests-wordpress --env-cwd=wp-content/plugin's/gutenberg pwd"
}
}On the other hand, a command like the one below works correctly on Linux, but cannot be processed on Windows because an empty string is passed to coerce. {
"scripts": {
"has-space-before": "wp-env run tests-wordpress --env-cwd=' wp content/plugins/gutenberg' pwd",
"has-space-after": "wp-env run tests-wordpress --env-cwd='wp content/plugins/gutenberg ' pwd"
}
} |
|
Thanks for the investigation @t-hamano. On the subject of the failures themselves, are these local terminal syntax errors or remote errors? I'm just curious whether or not the problem is the input being passed to |
|
Another approach: I made the trimming of spaces and quotes to be handled just before
If I understand correctly, this approach is what fixes the latter, right? |
Sounds good to me @t-hamano. The duplication of |
|
In b512390, I made the transformation to be performed directly at the point where the paths are concatenated. I may not be understanding your intent correctly, but is this correct? |
ObliviousHarmony
approved these changes
Dec 4, 2023
There was a problem hiding this comment.
That's perfect @t-hamano. I might suggest a comment explaining what the transformation is for but this works.
Thanks!
Indeed, I added a comment 👍 |
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 PR fixes an issue where scripts that include the
--env-cwdoption fail when executed on Windows OS. This allows commands such aslint:phpandformat:phpto work correctly on Windows OS.Why?
For example, when you run
lint:php, theother:update-packages.phpscript is executed before that. When running this script, the following error occurs on Windows OS:Notice that the path includes single quotes (
"/var/www/html/'wp-content/plugins/gutenberg'"). I don't know the exact cause, but I suspect that single quotes are handled differently in Windows OS.How?
I have confirmed that both formats below work on Windows OS. Although I don't have a strong opinion on the format, I chose the first option.
"test": "wp-env run --env-cwd=\"wp-content/plugins/gutenberg\" cli composer update --no-interaction"(escape double quote)"test": "wp-env run --env-cwd=wp-content/plugins/gutenberg cli composer update --no-interaction"(don't use quote)Testing Instructions
I only have a Windows machine, but I would be happy if you could confirm that commands such as
lint:phpandformat:phpwork as usual on Linux and MacOS.