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

allow rustdoc-js tests to be run at stage0 #135375

Merged
merged 1 commit into from
Jan 12, 2025
Merged

allow rustdoc-js tests to be run at stage0 #135375

merged 1 commit into from
Jan 12, 2025

Conversation

lolbinarycat
Copy link
Contributor

this mirrors the behavior of rustdoc-js-std tests.

previously this required COMPILETEST_FORCE_STAGE0.

@lolbinarycat
allow rustdoc-js tests to be run at stage0
af2247c 
this mirrors the behavior of rustdoc-js-std tests.

previously this required COMPILETEST_FORCE_STAGE0.
@rustbot
Copy link
Collaborator

rustbot commented Jan 11, 2025

r? @jieyouxu

rustbot has assigned @jieyouxu.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Jan 11, 2025
@jieyouxu
Copy link
Member

jieyouxu commented Jan 11, 2025 •
edited
Loading

Are rustdoc-js tests guaranteed to work against stage0 compiler/std?

One of the reasons run-make tests require COMPILETEST_FORCE_STAGE0 to run at --stage 0 is that they are not guaranteed to pass against a stage0 sysroot (w/ compiler/std).

In this configuration, wouldn't rustdoc-js test be running against a possibly-beta rustdoc? IOW, if rustdoc-js tests are not guaranteed to pass against a possibly-beta stage 0 rustdoc, then I think needing COMPILETEST_FORCE_STAGE0=1 if you request ./x test rustdoc-js --stage 0 for tests that aren't guaranteed to pass seems like a feature, not a bug.

cc @rust-lang/rustdoc, maybe you could provide some insights for this workflow?

@jieyouxu jieyouxu added T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 11, 2025
@lolbinarycat
Copy link
Contributor Author

a possibly-beta stage 0 rustdoc

stage 0 rustdoc is the rustdoc built with the stage0 compiler. it is not beta rustdoc

https://rustc-dev-guide.rust-lang.org/building/bootstrapping/what-bootstrapping-does.html#overview-1

@jieyouxu
Copy link
Member

... Right. Let me double-check in a bit.

@lolbinarycat
Copy link
Contributor Author

also, if the behavior of rustdoc-js was correct, then the behavior of rustdoc-js-std would be incorrect.

@jieyouxu
Copy link
Member

Oh wait, the step corresponding to rustdoc-js is actually RustdocJSNotStd. Okay, that seems very reasonable. Thanks for the clarifications.

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Jan 11, 2025

📌 Commit af2247c has been approved by jieyouxu

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 11, 2025
@jieyouxu
Copy link
Member

Slightly unrelated but, apparently the test mode is called js-doc-test, but the test suite is called rustdoc-js, and this test mode isn't used for any other test suites, lol?

@lolbinarycat
Copy link
Contributor Author

yeah i was surprised to see that rustdoc-js and rustdoc-js-std seem to use entirely different code paths instead of just calling into the same function with different arguments.

@jieyouxu
Copy link
Member

jieyouxu commented Jan 11, 2025 •
edited
Loading

I think there's also some logic that checks for a prefix rustdoc-js, but that matches rustdoc-json as well... 😅

@lolbinarycat
Copy link
Contributor Author

I just read that code, and I thought it actually checked for a suffix of rustdoc-js... which is basically just a string equality check with a footgun attached.

unless there's separate code that checks for a prefix that i didn't see.

@jieyouxu
Copy link
Member

No you're right, it checks for suffix (but why?), for some reason I thought it did prefix matching

@lolbinarycat
Copy link
Contributor Author

probably because prefix matching would make at least a little sense :p

most likely seems like a typo

@lolbinarycat
Copy link
Contributor Author

I could try to clean that up, if you want.

@jieyouxu
Copy link
Member

jieyouxu commented Jan 11, 2025 •
edited
Loading

I could try to clean that up, if you want.

Yeah, feel free to send a PR to rename the test mode (and use exact matching over suffix matching, probably), I'd happily review that.

@lolbinarycat
Copy link
Contributor Author

is it weird to put "rustdoc-js" in a string constant? i don't really like strings as enums due to their ability to promote typos into logic errors.

@lolbinarycat
Copy link
Contributor Author

technically also means less work for the constant deduplication pass, but i don't think that's worth optimizing for in 2025 :p

@jieyouxu
Copy link
Member

jieyouxu commented Jan 11, 2025 •
edited
Loading

is it weird to put "rustdoc-js" in a string constant? i don't really like strings as enums due to their ability to promote typos into logic errors.

Yes, I would've used enums. It's fine in the current iteration of compiletest (to fix this specific strangeness). If we're going to change how the suites and modes are propagated it should be done uniformally.

@lolbinarycat
Copy link
Contributor Author

I'm not sure if you're saying i should or should not use a string constant, tbh. inlining a constant is trivial tho :p

jhpratt added a commit to jhpratt/rust that referenced this pull request Jan 12, 2025
@jhpratt
Rollup merge of rust-lang#135375 - lolbinarycat:bootstrap-allow-stage…
a029084 
…0-rustdoc-js, r=jieyouxu

allow rustdoc-js tests to be run at stage0

this mirrors the behavior of rustdoc-js-std tests.

previously this required COMPILETEST_FORCE_STAGE0.
@jhpratt jhpratt mentioned this pull request Jan 12, 2025
Closed
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 12, 2025
@bors
Auto merge of rust-lang#135388 - jhpratt:rollup-zeyd0r8, r=jhpratt
3a49e62 
Rollup of 5 pull requests

Successful merges:

 - rust-lang#132232 (CI: build FreeBSD artifacts on FreeBSD 13.4)
 - rust-lang#135266 (Remove emsdk version update from 1.84.0 relnotes)
 - rust-lang#135364 (Cleanup `suggest_binding_for_closure_capture_self` diag in borrowck)
 - rust-lang#135375 (allow rustdoc-js tests to be run at stage0)
 - rust-lang#135379 (Make (unstable API) `UniqueRc` invariant for soundness)

r? `@ghost`
`@rustbot` modify labels: rollup
@matthiaskrgr matthiaskrgr mentioned this pull request Jan 12, 2025
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 12, 2025
@bors
Auto merge of rust-lang#135396 - matthiaskrgr:rollup-zublg1c, r=matth…
1b41e84 
…iaskrgr

Rollup of 5 pull requests

Successful merges:

 - rust-lang#135266 (Remove emsdk version update from 1.84.0 relnotes)
 - rust-lang#135364 (Cleanup `suggest_binding_for_closure_capture_self` diag in borrowck)
 - rust-lang#135375 (allow rustdoc-js tests to be run at stage0)
 - rust-lang#135379 (Make (unstable API) `UniqueRc` invariant for soundness)
 - rust-lang#135389 (compiletest: include stage0-sysroot libstd dylib in recipe dylib search path)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 6fa92ea into rust-lang:master Jan 12, 2025
6 checks passed
@rustbot rustbot added this to the 1.86.0 milestone Jan 12, 2025
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jan 12, 2025
@rust-timer
Unrolled build for rust-lang#135375
f103207 
Rollup merge of rust-lang#135375 - lolbinarycat:bootstrap-allow-stage0-rustdoc-js, r=jieyouxu

allow rustdoc-js tests to be run at stage0

this mirrors the behavior of rustdoc-js-std tests.

previously this required COMPILETEST_FORCE_STAGE0.
@lolbinarycat
Copy link
Contributor Author

now that the name of the test mode has been renamed, this no longer works!

this is why i don't like having a bunch of PRs open at once.

@jieyouxu jieyouxu mentioned this pull request Jan 17, 2025
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 17, 2025
@bors
Auto merge of rust-lang#135605 - jieyouxu:temp-require-force-rustdoc-…
bb3fa4b 
…js, r=clubby789

bootstrap: still require `COMPILETEST_FORCE_STAGE0` for `./x test rustdoc-js --stage 0`

This PR reverts rust-lang#135375, because through some more testing I found out `./x test rustdoc-js --stage 0` does not in fact build rustdoc, and all the tests fail. This can't be intended behavior, so at least require `COMPILETEST_FORCE_STAGE0` to make it less likely to run `rustdoc-js --stage 0` by accident.

The problem that `--stage 0` is not working at all for this rustdoc-js test suite is tracked over at rust-lang#135603.

cc `@lolbinarycat`

r? bootstrap
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@jieyouxu jieyouxu

Labels
A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.86.0
Development

Successfully merging this pull request may close these issues.
4 participants
@lolbinarycat @rustbot @jieyouxu @bors