-
Notifications
You must be signed in to change notification settings - Fork 12.1k
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
Migrate run-make/rustdoc-io-error
to rmake.rs
#124807
base: master
Are you sure you want to change the base?
Migrate run-make/rustdoc-io-error
to rmake.rs
#124807
Conversation
Some changes occurred in run-make tests. cc @jieyouxu |
7a43b72
to
182fbf7
Compare
182fbf7
to
74475c9
Compare
permissions.set_readonly(false); | ||
fs::set_permissions(&out_dir, permissions).unwrap(); | ||
|
||
#[cfg(not(windows))] |
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 there a reason assert_eq!(output.status.code().unwrap(), 1);
is non-Windows? Basic r/w permissions are available on Windows too right? Does rustdoc behave differently on Windows for a non-modifiable output directory?
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.
Because I couldn't ensure the exact error code value on Windows (I don't have windows) so for now I only check that it fails whatever the OS, and if it's unix, then I check the error code is 1.
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.
You could try editing PR CI to enable some Windows runners in PR CI and see if the error code is the same on Windows
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.
The original test was running fine on Windows so I suppose it's fine to just check the error code whatever the platform.
74475c9
to
6a614ad
Compare
Removed the |
Actually, the original test never ran on Windows 😄
|
Note for myself: should reduce coding while attending conferences. ^^' Let's make a trybuild to ensure it's ok, and otherwise let's see what windows actually return. @bors try |
…ror, r=<try> Migrate `run-make/rustdoc-io-error` to `rmake.rs` Part of rust-lang#121876. r? `@jieyouxu`
☀️ Try build successful - checks-actions |
Seems like windows is happy so let's go! :D @bors r=jieyouxu rollup |
☔ The latest upstream changes (presumably #124916) made this pull request unmergeable. Please resolve the merge conflicts. |
6a614ad
to
ab1a67e
Compare
Rebased. @bors r=jieyouxu rollup |
…error, r=jieyouxu Migrate `run-make/rustdoc-io-error` to `rmake.rs` Part of rust-lang#121876. r? `@jieyouxu`
💔 Test failed - checks-actions |
The backtrace (lol):
|
Right, you'll have to set backtrace on |
Let's put it everywhere. 😈 |
a4ca364
to
24580e8
Compare
@bors try |
…ror, r=<try> Migrate `run-make/rustdoc-io-error` to `rmake.rs` Part of rust-lang#121876. r? `@jieyouxu` try-job: armhf-gnu
This comment has been minimized.
This comment has been minimized.
💔 Test failed - checks-actions |
So there is no other output. It just succeeds at rendering docs in a read-only folder. Dark magic. |
24580e8
to
ff47f47
Compare
Seems like I removed some improvements I did to the permissions setting. Should be fixed. Let's see the ICE this time. @bors try |
…ror, r=<try> Migrate `run-make/rustdoc-io-error` to `rmake.rs` Part of rust-lang#121876. r? `@jieyouxu` try-job: armhf-gnu
The job Click to see the possible cause of the failure (guessed by this bot)
|
💔 Test failed - checks-actions |
Still same outcome. I have no idea what's wrong with this target... |
This comment was marked as off-topic.
This comment was marked as off-topic.
rustdoc isn't ICEing, it's the panic/assert from |
I'm a complete idiot, I saw
|
In that case, I think it's reasonable to change the assertion to not match on exit code == 1, but instead just check that exit code != 101 (for rustdoc's exit code). |
I'm not sure it will work since we want to ensure rustdoc returned an error, but worth trying I suppose... |
☔ The latest upstream changes (presumably #125254) made this pull request unmergeable. Please resolve the merge conflicts. |
Part of #121876.
r? @jieyouxu
try-job: armhf-gnu