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.

Sign up for GitHub

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

Jump to bottom

Revert "tools: update doc dependencies" #50414

Merged
merged 1 commit into from
Oct 26, 2023
Merged

Revert "tools: update doc dependencies" #50414

merged 1 commit into from
Oct 26, 2023

Conversation

richardlau
Copy link
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.

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/web-infra

@nodejs-github-bot 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 richardlau added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 26, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 26, 2023
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/55258/

@richardlau richardlau mentioned this pull request Oct 26, 2023
Merged
@richardlau richardlau added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 26, 2023
@joyeecheung joyeecheung added the fast-track PRs that do not need to wait for 48 hours to land. label Oct 26, 2023
@joyeecheung
Copy link
Member

joyeecheung commented Oct 26, 2023 •
edited
Loading

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

@github-actions
Copy link
Contributor

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

@richardlau
Copy link
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.)

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/55260/

@anonrig anonrig added the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 26, 2023
@nodejs-github-bot 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 nodejs-github-bot merged commit 67b1383 into nodejs:main Oct 26, 2023
64 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 67b1383

@richardlau richardlau deleted the revert-doc-deps branch October 26, 2023 20:48
@joyeecheung
Copy link
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

@joyeecheung
Copy link
Member

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

@joyeecheung
Copy link
Member

joyeecheung commented Oct 27, 2023 •
edited
Loading

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.

@lpinca lpinca mentioned this pull request Oct 29, 2023
Merged
alexfernandez pushed a commit to alexfernandez/node that referenced this pull request Nov 1, 2023
@richardlau @alexfernandez
Revert "tools: update doc dependencies"
0801253 
This reverts commit 6431c65.

PR-URL: nodejs#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>
targos pushed a commit that referenced this pull request Nov 11, 2023
@richardlau @targos
Revert "tools: update doc dependencies"
92d6403 
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>
@targos targos mentioned this pull request Nov 12, 2023
Merged
UlisesGascon pushed a commit that referenced this pull request Dec 11, 2023
@richardlau @UlisesGascon
Revert "tools: update doc dependencies"
7d43c5a 
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 UlisesGascon mentioned this pull request Dec 12, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marco-ippolito marco-ippolito marco-ippolito approved these changes

@aduh95 aduh95 aduh95 approved these changes

@joyeecheung joyeecheung joyeecheung approved these changes

@tniessen tniessen tniessen approved these changes

@anonrig anonrig anonrig approved these changes

@lpinca lpinca lpinca approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, 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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@richardlau @nodejs-github-bot @joyeecheung @lpinca @anonrig @tniessen @aduh95 @marco-ippolito