FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Revert "tools: update doc dependencies" by richardlau · Pull Request #50414 · nodejs/node · GitHub

/ node Public

Revert "tools: update doc dependencies" - #50414

Merged
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
richardlau:revert-doc-deps
Oct 26, 2023
Merged

Revert "tools: update doc dependencies"#50414
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
richardlau:revert-doc-deps

Conversation

Copy link
Copy Markdown
Member

This reverts commit 6431c65.

Refs: #49988 (comment)


This is a quick revert to fix the without-intl builds on the Jenkins CI. If someone else has a better fix that can be landed quickly, feel free to close this one.

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/web-infra

nodejs-github-bot added doc Issues and PRs related to the documentations. tools Issues and PRs related to the tools directory. labels Oct 26, 2023
richardlau added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 26, 2023
github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 26, 2023

Copy link
Copy Markdown
Collaborator

richardlau added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Oct 26, 2023
joyeecheung added the fast-track PRs that do not need to wait for 48 hours to land. label Oct 26, 2023

joyeecheung commented Oct 26, 2023
edited
Loading

Copy link
Copy Markdown
Member

Separated from this (which I think should land ASAP), I wonder if we should just skip doc generation/tests in without-intl builds

Copy link
Copy Markdown
Contributor

Fast-track has been requested by @joyeecheung. Please 👍 to approve.

Copy link
Copy Markdown
Member Author

Separated from this (which I think should land ASAP), I wonder if we should just skip doc generation/tests in without-intl builds

That was a suggestion in #35942 (comment), which references #41091 which I'm now confused over as it suggests we shouldn't be attempting to generate docs without intl available? Maybe either something was missed there or we've subsequently changed something in how the docs are generated that means they're no longer being skipped? 🤔
(My recommendation would be to land this to unbreak the CI and then investigate separately.)

Copy link
Copy Markdown
Collaborator

anonrig added the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 26, 2023
nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 26, 2023
nodejs-github-bot merged commit 67b1383 into nodejs:main Oct 26, 2023

Copy link
Copy Markdown
Collaborator

Landed in 67b1383

richardlau deleted the revert-doc-deps branch October 26, 2023 20:48

Copy link
Copy Markdown
Member

Maybe either something was missed there or we've subsequently changed something in how the docs are generated that means they're no longer being skipped?

Do we have other Node.js installations available in the system? #41091 checks the availability of intl using whatever Node.js binary available in the system (out/Release/node just takes precedence). If somehow there's a node executable with Intl on the path, that check could be invalid

Copy link
Copy Markdown
Member

hmm, I think this is caused by a missing dependency check in test/addons/.docbuildstamp

joyeecheung commented Oct 27, 2023
edited
Loading

Copy link
Copy Markdown
Member

So it seems the cause is that:

  1. We parse addons.md to generate addon tests
  2. The parser used to parse addons.md depends on i18n to work (via remark-gfm?)

Not sure how this can be addressed easily. I think we do still want to test the addons in addons.md in without-intl builds. Perhaps we can just use some dumb marker in addons.md to denote where the code examples are and write a custom parser instead of using remark.

targos pushed a commit that referenced this pull request Nov 11, 2023
This reverts commit 6431c65.

PR-URL: #50414
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
UlisesGascon pushed a commit that referenced this pull request Dec 11, 2023
This reverts commit 6431c65.

PR-URL: #50414
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
UlisesGascon mentioned this pull request Dec 12, 2023
This file contains hidden or 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author ready PRs that have at least one approval, no outstanding review comments, and a CI started. doc Issues and PRs related to the documentations. fast-track PRs that do not need to wait for 48 hours to land. tools Issues and PRs related to the tools directory.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants


Back | FazBrowse Home | New Git URL