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 npm run docs:build on Windows environments #16029

Merged
merged 5 commits into from
Jul 23, 2019

Conversation

noisysocks
Copy link
Member

Windows environments do not support shebang (#!) scripts. This caused npm run docs:build to error when running it on Windows, see #15956 (comment).

The fix is to run node docs/tool/update-data.js instead of running ./docs/tool/update-data.js.

I tested this by starting a Windows environment in VirtualBox, installing Node.js, downloading Gutenberg, and then running:

npm install
npm run docs:build

The command exited without error.

@noisysocks noisysocks added [Type] Developer Documentation Documentation for developers [Type] Build Tooling Issues or PRs related to build tooling labels Jun 7, 2019
@noisysocks noisysocks requested a review from oandregal June 7, 2019 04:19
@noisysocks noisysocks added the [Type] Bug An existing feature does not function as intended label Jun 7, 2019
@jg314
Copy link
Contributor

jg314 commented Jun 7, 2019

Thanks a lot for taking a look at this issue @noisysocks. For what it's worth, I made changes to the two files you provided here and ran npm run docs:build. Unfortunately, it still didn't work. Here's the error information in the logs:

1 verbose cli   'run',
1 verbose cli   'docs:build' ]
2 info using npm@6.9.0
3 info using node@v10.16.0
4 verbose run-script [ 'predocs:build', 'docs:build', 'postdocs:build' ]
5 info lifecycle gutenberg@5.8.0~predocs:build: gutenberg@5.8.0
6 info lifecycle gutenberg@5.8.0~docs:build: gutenberg@5.8.0
7 verbose lifecycle gutenberg@5.8.0~docs:build: unsafe-perm in lifecycle true
8 verbose lifecycle gutenberg@5.8.0~docs:build: PATH: C:\Program Files\nodejs\node_modules\npm\node_modules\npm-lifecycle\node-gyp-bin;C:\Users\username_removed\Local Sites\wordpress-testing\app\public\wp-content\plugins\gutenberg\node_modules\.bin;C:\Program Files (x86)\Intel\iCLS Client\;C:\Program Files\Intel\iCLS Client\;C:\Windows\system32;C:\Windows;C:\Windows\System32\Wbem;C:\Windows\System32\WindowsPowerShell\v1.0\;C:\Program Files (x86)\Intel\Intel(R) Management Engine Components\DAL;C:\Program Files\Intel\Intel(R) Management Engine Components\DAL;C:\Program Files (x86)\Intel\Intel(R) Management Engine Components\IPT;C:\Program Files\Intel\Intel(R) Management Engine Components\IPT;C:\Program Files (x86)\NVIDIA Corporation\PhysX\Common;C:\WINDOWS\system32;C:\WINDOWS;C:\WINDOWS\System32\Wbem;C:\WINDOWS\System32\WindowsPowerShell\v1.0\;C:\WINDOWS\System32\OpenSSH\;C:\Program Files\Git\cmd;C:\Program Files (x86)\Windows Kits\8.1\Windows Performance Toolkit\;C:\Program Files\TortoiseSVN\bin;C:\Program Files\nodejs\;C:\Ruby25-x64\bin;C:\Users\username_removed\AppData\Local\Microsoft\WindowsApps;C:\Users\username_removed\AppData\Local\GitHubDesktop\bin;C:\Users\username_removed\AppData\Local\Programs\Microsoft VS Code\bin;C:\Users\username_removed\AppData\Local\Microsoft\WindowsApps;C:\Users\username_removed\AppData\Roaming\npm
9 verbose lifecycle gutenberg@5.8.0~docs:build: CWD: C:\Users\jonat\Local Sites\wordpress-testing\app\public\wp-content\plugins\gutenberg
10 silly lifecycle gutenberg@5.8.0~docs:build: Args: [ '/d /s /c', 'node ./docs/tool && node ./bin/update-readmes' ]
11 silly lifecycle gutenberg@5.8.0~docs:build: Returned: code: 1  signal: null
12 info lifecycle gutenberg@5.8.0~docs:build: Failed to exec docs:build script
13 verbose stack Error: gutenberg@5.8.0 docs:build: `node ./docs/tool && node ./bin/update-readmes`
13 verbose stack Exit status 1
13 verbose stack     at EventEmitter.<anonymous> (C:\Program Files\nodejs\node_modules\npm\node_modules\npm-lifecycle\index.js:301:16)
13 verbose stack     at EventEmitter.emit (events.js:198:13)
13 verbose stack     at ChildProcess.<anonymous> (C:\Program Files\nodejs\node_modules\npm\node_modules\npm-lifecycle\lib\spawn.js:55:14)
13 verbose stack     at ChildProcess.emit (events.js:198:13)
13 verbose stack     at maybeClose (internal/child_process.js:982:16)
13 verbose stack     at Process.ChildProcess._handle.onexit (internal/child_process.js:259:5)
14 verbose pkgid gutenberg@5.8.0
15 verbose cwd C:\Users\username_removed\Local Sites\wordpress-testing\app\public\wp-content\plugins\gutenberg
16 verbose Windows_NT 10.0.17763
17 verbose argv "C:\\Program Files\\nodejs\\node.exe" "C:\\Program Files\\nodejs\\node_modules\\npm\\bin\\npm-cli.js" "run" "docs:build"
18 verbose node v10.16.0
19 verbose npm  v6.9.0
20 error code ELIFECYCLE
21 error errno 1
22 error gutenberg@5.8.0 docs:build: `node ./docs/tool && node ./bin/update-readmes`
22 error Exit status 1
23 error Failed at the gutenberg@5.8.0 docs:build script.
23 error This is probably not a problem with npm. There is likely additional logging output above.
24 verbose exit [ 1, true ]

I'm not entirely sure if this is related, but wanted to mention it here in case it's helpful.

Verified

This commit was created on github.com and signed with GitHub’s verified signature. The key has expired.
Windows environments do not support shebang (#!) scripts. This caused
`npm run docs:build` to error when running it on Windows. The fix is to
run `node docs/tool/update-data.js` instead of running
`./docs/tool/update-data.js`.

Similarly, running `node path/to/directory` can cause problems, so we
update the docs:build task to use `node path/to/directory/index.js`.

docs:build: Use the .js extension in the filename
@noisysocks noisysocks force-pushed the fix/building-docs-on-windows branch from 96e2daa to 4e37751 Compare July 4, 2019 01:20
@noisysocks
Copy link
Member Author

Hey @jg314! It's not totally clear what the error is. I suspect it maybe is to do with us calling node ./docs/tool instead of the fully qualified node ./docs/tool/index.js. I've updated the PR with this change. Could you try it again?

@noisysocks
Copy link
Member Author

I'd very much like to be able to re-create the errors you're getting on my own Windows testing environment. What version of Windows are you running, @jg314?

@jg314
Copy link
Contributor

jg314 commented Jul 5, 2019

Thanks a ton for troubleshooting this with me @noisysocks. The good news on this front is that with your latest change npm run docs:build ran without any errors. The not so great news is that like I mentioned in #15956 (comment), a ton of the documentation is being wiped out. Here's some of the git diff:

diff --git a/docs/designers-developers/developers/data/data-core-block-editor.md b/docs/designers-developers/developers/data/data-core-block-editor.md
index 6e496446d..cebbb128c 100644
--- a/docs/designers-developers/developers/data/data-core-block-editor.md
+++ b/docs/designers-developers/developers/data/data-core-block-editor.md
@@ -8,812 +8,247 @@ Namespace: `core/block-editor`.
 
 <a name="canInsertBlockType" href="#canInsertBlockType">#</a> **canInsertBlockType**
 
-Determines if the given block type is allowed to be inserted into the block list.
-
-_Parameters_
-
--   _state_ `Object`: Editor state.
--   _blockName_ `string`: The name of the block type, e.g.' core/paragraph'.
--   _rootClientId_ `?string`: Optional root client ID of block list.
-
-_Returns_
-
--   `boolean`: Whether the given block type is allowed to be inserted.
+Undocumented declaration.
 
 <a name="getAdjacentBlockClientId" href="#getAdjacentBlockClientId">#</a> **getAdjacentBlockClientId**
 
-Returns the client ID of the block adjacent one at the given reference
-startClientId and modifier directionality. Defaults start startClientId to
-the selected block, and direction as next block. Returns null if there is no
-adjacent block.
-
-_Parameters_
-
--   _state_ `Object`: Editor state.
--   _startClientId_ `?string`: Optional client ID of block from which to search.
--   _modifier_ `?number`: Directionality multiplier (1 next, -1 previous).
-
-_Returns_
-
--   `?string`: Return the client ID of the block, or null if none exists.
+Undocumented declaration.
 
 <a name="getBlock" href="#getBlock">#</a> **getBlock**
 
-Returns a block given its client ID. This is a parsed copy of the block,
-containing its `blockName`, `clientId`, and current `attributes` state. This
-is not the block's registration settings, which must be retrieved from the
-blocks module registration store.
-
-_Parameters_
-
--   _state_ `Object`: Editor state.
--   _clientId_ `string`: Block client ID.
-
-_Returns_
-
--   `Object`: Parsed block object.
+Undocumented declaration.
 
 <a name="getBlockAttributes" href="#getBlockAttributes">#</a> **getBlockAttributes**
 
-Returns a block's attributes given its client ID, or null if no block exists with
-the client ID.
-
-_Parameters_
-
--   _state_ `Object`: Editor state.
--   _clientId_ `string`: Block client ID.
-
-_Returns_
-
--   `?Object`: Block attributes.
+Undocumented declaration.
 
 <a name="getBlockCount" href="#getBlockCount">#</a> **getBlockCount**
 
-Returns the number of blocks currently present in the post.
-
-_Parameters_
-
--   _state_ `Object`: Editor state.
--   _rootClientId_ `?string`: Optional root client ID of block list.
-
-_Returns_
-
--   `number`: Number of blocks in the post.
+Undocumented declaration.

In case you need it, here are the details of my setup:

  • OS: Windows 10 Home
  • Node Version: 10.16.0
  • NPM Version: 6.9.0

Both Node and NPM were installed directly on Windows, not using Windows Subsystem for Linux.

Let me know if there is anything else you think I can do at this point. Thanks again.

Verified

This commit was created on github.com and signed with GitHub’s verified signature. The key has expired.
This allows `npm run docs:build` to generate docs on Windows
environments.
@noisysocks
Copy link
Member Author

Hey @jg314, thanks for the extra info. I think I found the source of the bug and have pushed aa04d5d to fix it. I've tested running npm run docs:build in my Windows testing environment and confirmed that it doesn't delete any existing documentation. Let me know how it goes for you.

@noisysocks
Copy link
Member Author

@nosolosw: Would love your 👀s on this one!

@jg314
Copy link
Contributor

jg314 commented Jul 10, 2019

That worked @noisysocks! I was able to run npm run docs:build without errors and without wiping out any other documentation. Thanks a ton for all the help getting this working.

@noisysocks
Copy link
Member Author

Glad to hear it! This PR should be ready for merge once it gets a technical review, then 🙂

@noisysocks noisysocks added this to the Gutenberg 6.2 milestone Jul 11, 2019

Verified

This commit was created on github.com and signed with GitHub’s verified signature. The key has expired.

Verified

This commit was created on github.com and signed with GitHub’s verified signature. The key has expired.
so it works consistently in Windows machines.

Verified

This commit was created on github.com and signed with GitHub’s verified signature. The key has expired.
@noisysocks noisysocks requested review from aduth, nerrad and ntwb as code owners July 17, 2019 09:58
@oandregal
Copy link
Member

Hey, sorry for the delay to respond (I was AFK).

I took the liberty to push a couple of minor changes for consistency and to make sure this also works when the tasks are executed via git pre-commit hook.

@jg314 would you mind testing these 3 scenarios?

  1. npm run docs:build in this branch (no changes).
  2. Test that it works when there are changes to docs. For example:
  • Edit the packages/dom/src/dom.js file and modify the JSDoc comments for the wrap function at the botton.
  • Any change will do, I modified the description of one of the parameters.
  • Commit the changes.
  • Verify that the packages/dom/README.md has those changes.
  1. Test that it works when there are no changes to docs. For example:
  • Edit the packages/dom/src/dom.js file and remove the Browser dependencies comment at the top.
  • Commit the changes.
  • Verify that there are no changes.

I think this is ready to go, I just want to make sure everything is fine after the changes I made.

@jg314
Copy link
Contributor

jg314 commented Jul 18, 2019

I'm happy to test it @nosolosw. Give me a few days and I'll take a look at it.

@jg314
Copy link
Contributor

jg314 commented Jul 19, 2019

@nosolosw, unfortunately, it didn't work. Step 1 to run npm run docs:build with no changes worked as expected. For step 2 I made a change to one of the JSDoc comments in packages/dom/src/dom.js file and committed it. When I did that a long list of errors showed up that looks like this:

× wp-scripts lint-js found some errors. Please fix them and try committing again.

C:\directory_name_here\gutenberg\packages\dom\src\dom.js
  1:4    error  Expected linebreaks to be 'LF' but found 'CRLF'           linebreak-style
  2:25   error  Expected linebreaks to be 'LF' but found 'CRLF'           linebreak-style
  3:4    error  Expected linebreaks to be 'LF' but found 'CRLF'           linebreak-style
.....
✖ 672 problems (672 errors, 0 warnings)
672 errors and 0 warnings potentially fixable with the `--fix` option.

I'm not sure if this matters to you all, but I figured it's worth mentioning that Step 1 I mentioned above only works if none of the directories where this code lives has a space in it. If it does, running npm run docs:build generates the following errors. I only bring this up because Local by Flywheel, the tool I use for WordPress development, by default puts the sites in a folder with a space in it.

> gutenberg@6.0.0 docs:build C:\Users\myUsernameFiller\Desktop\gutenberg test
> node ./docs/tool/index.js && node ./bin/update-readmes.js


C:\Users\myUsernameFiller\Desktop\gutenberg test\docs\tool\update-data.js:48
                        throw stderr.toString();
                        ^
'C:\Users\myUsernameFiller\Desktop\gutenberg' is not recognized as an internal or external command,
operable program or batch file.

child_process.js:650
    throw err;
    ^

Error: Command failed: node C:\Users\myUsernameFiller\Desktop\gutenberg test\docs\tool\update-data.js

C:\Users\myUsernameFiller\Desktop\gutenberg test\docs\tool\update-data.js:48
                        throw stderr.toString();
                        ^
'C:\Users\myUsernameFiller\Desktop\gutenberg' is not recognized as an internal or external command,
operable program or batch file.


    at checkExecSyncError (child_process.js:629:11)
    at execFileSync (child_process.js:647:13)
    at Object.<anonymous> (C:\Users\myUsernameFiller\Desktop\gutenberg test\docs\tool\index.js:18:1)
    at Module._compile (internal/modules/cjs/loader.js:776:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:787:10)
    at Module.load (internal/modules/cjs/loader.js:653:32)
    at tryModuleLoad (internal/modules/cjs/loader.js:593:12)
    at Function.Module._load (internal/modules/cjs/loader.js:585:3)
    at Function.Module.runMain (internal/modules/cjs/loader.js:829:12)
    at startup (internal/bootstrap/node.js:283:19)
npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! gutenberg@6.0.0 docs:build: `node ./docs/tool/index.js && node ./bin/update-readmes.js`
npm ERR! Exit status 1
npm ERR!
npm ERR! Failed at the gutenberg@6.0.0 docs:build script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

npm ERR! A complete log of this run can be found in:
npm ERR!     C:\directory_name_here\AppData\Roaming\npm-cache\_logs\2019-07-19T21_13_12_424Z-debug.log

Let me know what else I can do to help.

@oandregal
Copy link
Member

For step 2 I made a change to one of the JSDoc comments in packages/dom/src/dom.js file and committed it. When I did that a long list of errors showed

It sounds like there were linebreaks errors, unrelated to the doc changes.

May it be that your editor auto-updates the line endings due to some setting? Perhaps you might look into your editor settings or use a simple one for this test that doesn't do that kind of things (notepad?).

@jg314
Copy link
Contributor

jg314 commented Jul 19, 2019

It's definitely possible it was my editor. I'm using Visual Studio Code though so I would be a little nervous to move forward with a solution that doesn't function well with that editor. I'm not basing this on data, but I think it's one of the most popular editors on Windows these days.

@oandregal
Copy link
Member

You should work with whatever makes you comfortable! :)

I didn't mean you should change your editor forever, but to test this PR without modifying anything else than the JSDoc change. We want to isolate sources of error to make progress. There should be an option in VSCode to set EOL (end of lines) to LF in windows, that should suffice.

@jg314
Copy link
Contributor

jg314 commented Jul 20, 2019

Thanks @nosolosw. I definitely didn't think you were telling me to switch editors. I was just worried that if it didn't work for me, then it wouldn't work for anyone else on Windows using Visual Studio Code.

It actually turns out the issue was with my git configuration. I had to run this in the command line to stop git from replacing my line endings:

git config --global core.autocrlf false

After making that change I was able to work in Visual Studio Code without any issues and run through steps 1, 2 and 3 successfully.

The only item of note was that for step 2 packages/dom/README.md did change, but the change wasn't included in the initial commit. Instead, I had to add another commit for packages/dom/README.md. I figured that's probably intentional though.

The last item is the space in the directory names, but I understand if you'd rather not tackle that now.

Let me know what else I can do to help.

Copy link
Member

@oandregal oandregal left a comment

Choose a reason for hiding this comment

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

This is ready to go!

@noisysocks noisysocks merged commit 643412c into master Jul 23, 2019
@noisysocks noisysocks deleted the fix/building-docs-on-windows branch July 23, 2019 02:02
@noisysocks
Copy link
Member Author

Thanks all! ❤️

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 [Type] Developer Documentation Documentation for developers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants