-
Notifications
You must be signed in to change notification settings - Fork 13.2k
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
ci: Enable opt-dist for dist-aarch64-linux builds #133807
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -146,6 +146,21 @@ fn create_environment(args: Args) -> anyhow::Result<(Environment, Vec<String>)> | |
let target_triple = | ||
std::env::var("PGO_HOST").expect("PGO_HOST environment variable missing"); | ||
|
||
let is_aarch64 = target_triple.starts_with("aarch64"); | ||
|
||
let mut skip_tests = vec![ | ||
// Fails because of linker errors, as of June 2023. | ||
"tests/ui/process/nofile-limit.rs".to_string(), | ||
]; | ||
|
||
if is_aarch64 { | ||
skip_tests.extend([ | ||
// Those tests fail only inside of Docker on aarch64, as of December 2024 | ||
"tests/ui/consts/promoted_running_out_of_memory_issue-130687.rs".to_string(), | ||
"tests/ui/consts/large_const_alloc.rs".to_string(), | ||
]); | ||
} | ||
|
||
Comment on lines
+149
to
+163
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please, I beg of you, do not do this. This hack is causing CI to pass and local developer workflows to fail, and it is hiding this regression #135952. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To explain, we already skip some tests in a similar manner on dist x64 (not just the explicitly skipped ones, but also whole test suites, e.g. run-make). Some tests fail on dist, but work locally, these are candidates for being skipped on CI (after all, running the test suite on an extracted dist atchive is already a hack). But if the test fails also outside of this extracted dist setup, then it shouldn't be skipped, ofc. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have the same feedback about the two tests that were using this feature before. See #135961. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay, we could remove specific skipped tests, but do you also have an issue with skipping any tests in the dist tests? Because we currently don't run some parts of the test suite, so these are also effectivelly skipped, just without being explicitly enumerated. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Of the 4 tests that were individually skipped, 3 also did not work for me with Ignoring or skipping tests in the harness is fine if there is some fundamental incompatibility with the harness. Or not running them because nobody has gotten to wiring them into opt-dist yet. I would have preferred these tests be ignored in the tests themselves via annotations; that would have at least not had me chasing my tail wondering how CI could be passing when the tests fail locally. And it would have made the problem more visible. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah I agree with explicit skipping via annotations being the better approach for these kinds of problems, you're right. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll write up a patch to change this in a moment, let's see what people say There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point with the annotations. Recently we added a test annotation for only running a given test in dist jobs, the same should be usable also for ignoring a test in a dist job (#135164 - There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've posted a PR that I've linked above which fixes the two tests that aren't related to the aarch64 vs x86_64 allocation failure issue. I'm away from a keyboard for a few days, but it looks like it works, and I would recommend applying that change and ignoring the large const allocs failure on aarch64, pending further investigation. |
||
let checkout_dir = Utf8PathBuf::from("/checkout"); | ||
let env = EnvironmentBuilder::default() | ||
.host_tuple(target_triple) | ||
|
@@ -155,11 +170,9 @@ fn create_environment(args: Args) -> anyhow::Result<(Environment, Vec<String>)> | |
.artifact_dir(Utf8PathBuf::from("/tmp/tmp-multistage/opt-artifacts")) | ||
.build_dir(checkout_dir.join("obj")) | ||
.shared_llvm(true) | ||
.use_bolt(true) | ||
.skipped_tests(vec![ | ||
// Fails because of linker errors, as of June 2023. | ||
"tests/ui/process/nofile-limit.rs".to_string(), | ||
]) | ||
// FIXME: Enable bolt for aarch64 once it's fixed upstream. Broken as of December 2024. | ||
.use_bolt(!is_aarch64) | ||
.skipped_tests(skip_tests) | ||
.build()?; | ||
|
||
(env, shared.build_args) | ||
|
@@ -304,7 +317,8 @@ fn execute_pipeline( | |
// the final dist build. However, when BOLT optimizes an artifact, it does so *in-place*, | ||
// therefore it will actually optimize all the hard links, which means that the final | ||
// packaged `libLLVM.so` file *will* be BOLT optimized. | ||
bolt_optimize(&llvm_lib, &llvm_profile).context("Could not optimize LLVM with BOLT")?; | ||
bolt_optimize(&llvm_lib, &llvm_profile, env) | ||
.context("Could not optimize LLVM with BOLT")?; | ||
|
||
let rustc_lib = io::find_file_in_dir(&libdir, "librustc_driver", ".so")?; | ||
|
||
|
@@ -319,7 +333,7 @@ fn execute_pipeline( | |
print_free_disk_space()?; | ||
|
||
// Now optimize the library with BOLT. | ||
bolt_optimize(&rustc_lib, &rustc_profile) | ||
bolt_optimize(&rustc_lib, &rustc_profile, env) | ||
.context("Could not optimize rustc with BOLT")?; | ||
|
||
// LLVM is not being cleared here, we want to use the BOLT-optimized LLVM | ||
|
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.
Either way this is a completely new dockerfile, so do you mean just replace this with a simple
./x dist
call and then wrap it with opt-dist separately? Just in separate commits or separate PRs altogether?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.
I meant a separate PR, so that we can land these two changes (move to aarch64 host first, and then enable optimizations) separately :)
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.
Makes sense, just wanted to make sure - no problem :)