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

Allow spaces in file paths for package build process #14789

Merged
merged 1 commit into from
Apr 6, 2019

Conversation

svenvanhal
Copy link
Contributor

@svenvanhal svenvanhal commented Apr 3, 2019

Description

Currently, the package build process fails when a path contains spaces, because the build script as well as the package location paths are not escaped in bin/packages/watch.js.

This PR escapes both paths, to support spaces.

Additional information

The following error is thrown (Windows):

node 'C:\Users\[path with spaces]\wp-content\plugins\gutenberg\bin\packages\build.js'
internal/modules/cjs/loader.js:584
    throw err;
    ^

Error: Cannot find module 'C:\Users\[username]\'C:\Users\[path up until space]'
    at Function.Module._resolveFilename (internal/modules/cjs/loader.js:582:15)
    at Function.Module._load (internal/modules/cjs/loader.js:508:25)
    at Function.Module.runMain (internal/modules/cjs/loader.js:754:12)
    at startup (internal/bootstrap/node.js:283:19)
    at bootstrapNodeJSCore (internal/bootstrap/node.js:622:3)

By adding double quotes to the BUILD_CMD variable in bin/packages/watch.js and also the argument provided to the command (the path to the package to build), spaces are now supported.

This is the only location in the code where a command gets constructed and executed "manually"; all other paths pass through node's fs which already supports spaces.

Considerations

Since spaces in paths are usually escaped with a backslash on *nix, but with double quotes on Windows, I've opted for the latter since that works across all platforms. There are other approaches available to solve this problem, but this is the most unobtrusive.

How has this been tested?

This branch was tested on Windows (build 1809) and MacOS Mojave 10.14.2, using paths with as well as without spaces in the build path.

Run npm run dev:packages, change a file and verify that the build process finishes without errors.

Types of changes

Bug fix (non-breaking change which fixes an issue)

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.

@gziolo gziolo added [Type] Build Tooling Issues or PRs related to build tooling Good First Review A PR that's suitable for someone looking to contribute for the first time by reviewing code labels Apr 3, 2019
Copy link
Contributor

@draganescu draganescu left a comment

Choose a reason for hiding this comment

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

I have tested locally and it works as described, both for paths with spaces and for paths without spaces. The solution is good since escaping spaces with \\ would raise more problems for the node command's file list.

@draganescu
Copy link
Contributor

@svenvanhal for some reason Travis fails, check that out, might be a false alarm :)

@draganescu draganescu removed the Good First Review A PR that's suitable for someone looking to contribute for the first time by reviewing code label Apr 3, 2019
@draganescu draganescu added this to the 5.5 (Gutenberg) milestone Apr 3, 2019
@svenvanhal
Copy link
Contributor Author

Yes, strange, the failing tests should be unrelated to the proposed change (I don't think the npm run dev commands are tested at all).

More PRs fail seemingly randomly at the moment: https://travis-ci.com/WordPress/gutenberg/pull_requests

@aduth
Copy link
Member

aduth commented Apr 3, 2019

There was some earlier discussion about the block transforms test in Slack (register). I'll try restarting the build, but otherwise it may require to rebase and force push the branch.

@gziolo gziolo added the [Type] Bug An existing feature does not function as intended label Apr 6, 2019
@gziolo gziolo merged commit a9f6a1e into WordPress:master Apr 6, 2019
@gziolo
Copy link
Member

gziolo commented Apr 6, 2019

Thanks for this fix. Travis should behave better now as we landed improvements to e2e tests and test helpers which should improve their reliability.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Bug An existing feature does not function as intended [Type] Build Tooling Issues or PRs related to build tooling
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants