-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Enforce rustdoc-gui test-suite run #84586
Enforce rustdoc-gui test-suite run #84586
Conversation
Yes, make sure nodejs and npm are installed on those builders. You can check by looking at the build logs for |
9000426
to
2f0bd74
Compare
This comment has been minimized.
This comment has been minimized.
Well, I guess now I have to add it in multiple places then. :3 |
Why are we putting this check on the PR builder? I understand conceptually that it's nice to get breakage early, but I worry that we shouldn't just add onto that builder without consideration. It may be worthwhile to take the approach of the tools builder, where we only run it directly on PRs with modification to the tools directory (while bors full builds always run it), and move the check from the aux builder to the tools builder. |
Changes to rustdoc almost never modify the tools directory, so that means this check won't run on PRs. |
Sure, we can add rustdoc to the list of things which trigger a build? |
Just to be sure: it's something to do on "infra side", not in this PR, right? |
@GuillaumeGomez no, it should go in this PR. You should find where |
I see, thanks for the clarification. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
c9fe2fc
to
1a44c00
Compare
xz-utils \ | ||
nodejs | ||
xz-utils | ||
|
||
RUN curl -sL https://nodejs.org/dist/v14.4.0/node-v14.4.0-linux-x64.tar.xz | tar -xJ | ||
ENV PATH="/node-v14.4.0-linux-x64/bin:${PATH}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is npm not available through apt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apparently not based on the previous error.
Another thing to be noted: if the |
I thought you changed it to give a hard error? If not, can you do that now? |
No, I only enforced the presence of |
1a44c00
to
3feb6ba
Compare
8a8c254
to
ab2dbbe
Compare
ab2dbbe
to
5096bd4
Compare
This comment has been minimized.
This comment has been minimized.
6b8a7c9
to
6b02464
Compare
This comment has been minimized.
This comment has been minimized.
6b02464
to
ff7a7f6
Compare
@Mark-Simulacrum Updated! |
ff7a7f6
to
5358498
Compare
@Mark-Simulacrum Seems like the CI wasn't run on rustdoc-GUI... EDIT: i was tired, it's normal because there is no rustdoc or rustdoc-GUI changes. So all normal. Waiting for @Mark-Simulacrum final review then I guess. :) |
@bors r+ |
📌 Commit 5358498 has been approved by |
☀️ Test successful - checks-actions |
Part of #84550