-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Rewrite separate-link
, separate-link-fail
and allocator-shim-circular-deps
run-make
tests to ui
or rmake
#126607
Conversation
This PR modifies cc @jieyouxu |
65932cd
to
184dcf2
Compare
This comment has been minimized.
This comment has been minimized.
184dcf2
to
11eded3
Compare
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.
Thanks, I left some review comments.
src/tools/tidy/src/ui_tests.rs
Outdated
@@ -15,7 +15,7 @@ use std::path::{Path, PathBuf}; | |||
const ENTRY_LIMIT: u32 = 900; | |||
// FIXME: The following limits should be reduced eventually. | |||
|
|||
const ISSUES_ENTRY_LIMIT: u32 = 1672; | |||
const ISSUES_ENTRY_LIMIT: u32 = 1673; |
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.
Problem: please don't add the test to ui/issues
because it's semantically meaningless, please find a more meaningful subdirectory.
// The compiler flags no-link (and by extension, link-only) used to be broken | ||
// due to changes in encoding/decoding. This was patched, and this test ensures | ||
// that these flags are not broken again, resulting in successful compilation. |
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.
Nit: this test is only a basic sanity check, it does not "ensure" unfortunately.
// This test is designed to intentionally introduce a circular dependency scenario to check | ||
// that a specific compiler bug doesn't make a resurgence. | ||
// The bug in question arose when at least one crate | ||
// required a global allocator, and that crate was placed after | ||
// the one defining it in the linker order. | ||
// The generated symbols.o should not result in any linker errors. | ||
// See https://github.com/rust-lang/rust/issues/112715 |
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.
Suggestion: actually, if this test and its auxillaries are sensitive to link order, let's keep it as a run-make test so we can explicitly control the link order.
@rustbot author |
11eded3
to
78998f3
Compare
@rustbot review |
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.
Thanks!
@bors r+ rollup=iffy (anything that touches linkage is always a bit sus) |
☀️ Test successful - checks-actions |
Finished benchmarking commit (4e63822): comparison URL. Overall result: no relevant changes - no action needed@rustbot label: -perf-regression Instruction countThis benchmark run did not return any relevant results for this metric. Max RSS (memory usage)Results (primary -4.0%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesResults (primary 0.7%, secondary 2.9%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 691.815s -> 690.013s (-0.26%) |
Part of #121876 and the associated Google Summer of Code project.