-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Run part of cg_clif's tests in CI #112701
Conversation
(rustbot has picked a reviewer for you, use r? to override) |
src/bootstrap/dist.rs
Outdated
let backends_dst = image.join("lib").join(&backends_rel); | ||
|
||
t!(fs::create_dir_all(&backends_dst)); | ||
builder.cp_r(&backends_src, &backends_dst); |
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 had to make this change immediately to prevent shipping cg_clif in the rustc component. #81746 will create a separate component for cg_clif.
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.
Doesn't this break users who do want this? If we avoid modifying defaults or change CI to not have cranelift enabled in dist builders, then this is probably not needed, right?
r? @Mark-Simulacrum |
config.example.toml
Outdated
@@ -608,7 +608,7 @@ changelog-seen = 2 | |||
# and currently the only standard options supported are `"llvm"`, `"cranelift"` | |||
# and `"gcc"`. The first backend in this list will be used as default by rustc | |||
# when no explicit backend is specified. | |||
#codegen-backends = ["llvm"] | |||
#codegen-backends = ["llvm", "cranelift"] |
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 list here tends to be the default built list - are we starting to build cranelift in this PR by default?
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.
Yes. We already compile it by default in ./x.py check
. If this was enabled in the CI config instead it would be easy to forget to test it locally.
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.
@@ -13,6 +13,8 @@ extended = true | |||
download-ci-llvm = false | |||
[rust] | |||
download-rustc = false | |||
# Skip building the cranelift backend. It isn't ready to be shipped yet. | |||
codegen-backends = ["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.
I don't think we can rely on profiles as the way to modify the defaults - is there a reason we're modifying those in this PR? We can enable building/testing cranelift in CI without changing defaults.
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.
لا أعتقد أنه يمكننا الاعتماد على الملفات الشخصية كطريقة لتعديل الإعدادات الافتراضية - هل هناك سبب لتعديل تلك الموجودة في هذه العلاقات العامة؟ يمكننا تمكين بناء / اختبار رافعة الرافعة في CI دون تغيير الإعدادات الافتراضية.
src/bootstrap/dist.rs
Outdated
let backends_dst = image.join("lib").join(&backends_rel); | ||
|
||
t!(fs::create_dir_all(&backends_dst)); | ||
builder.cp_r(&backends_src, &backends_dst); |
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.
Doesn't this break users who do want this? If we avoid modifying defaults or change CI to not have cranelift enabled in dist builders, then this is probably not needed, right?
|
||
if !builder.config.rust_codegen_backends.contains(&INTERNER.intern_str("cranelift")) { | ||
builder.info("cranelift not in rust.codegen-backends. skipping"); | ||
return; |
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 suspect these might want to be default_condition/lazy_default_condition rather than gated here...
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.
That would still attempt to run it on ./x.py test compiler/rustc_codegen_cranelift
, right?
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.
Yes, and allow running it that way even if rust_codegen_backends doesn't contain cranelift.
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.
librustc_codegen_cranelift-*.so needs to end up in the sysroot for cg_clif testing to work. This doesn't happen if rust_codegen_backends
doesn't contain cranelift
. And overwriting rust_codegen_backends
if ./x.py test compiler/rustc_codegen_cranelift
is done would lead to surprising results I think.
☔ The latest upstream changes (presumably #113102) made this pull request unmergeable. Please resolve the merge conflicts. |
c384140
to
fcd0a0c
Compare
fcd0a0c
to
e1eebff
Compare
This comment has been minimized.
This comment has been minimized.
e1eebff
to
b9dd977
Compare
This comment has been minimized.
This comment has been minimized.
aa5c1fb
to
3c2fc10
Compare
I switched everything to enable cg_clif testing in ci/run.sh rather than by enabling cg_clif by default for all |
@rustbot ready I think that this is ready for another round of review. |
r=me with commits squashed |
6c8c5cb
to
238d8e3
Compare
Rebased and squashed. @bors r=Mark-Simulacrum |
☀️ Test successful - checks-actions |
Finished benchmarking commit (c0583a0): 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)ResultsThis 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.
CyclesThis benchmark run did not return any relevant results for this metric. Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 629.704s -> 630.509s (0.13%) |
Run part of `rustc_codegen_gcc`'s tests in CI Thanks to rust-lang#112701 and `@bjorn3,` it made this much easier. Also cc `@antoyo.` r? `@bjorn3`
Run part of `rustc_codegen_gcc`'s tests in CI Thanks to rust-lang#112701 and `@bjorn3,` it made this much easier. Also cc `@antoyo.` r? `@bjorn3`
…ozkan Run part of `rustc_codegen_gcc`'s tests in CI Thanks to rust-lang#112701 and `@bjorn3,` it made this much easier. Also cc `@antoyo.` r? `@bjorn3`
…ozkan Run part of `rustc_codegen_gcc`'s tests in CI Thanks to rust-lang#112701 and `@bjorn3,` it made this much easier. Also cc `@antoyo.` r? `@bjorn3`
…ozkan Run part of `rustc_codegen_gcc`'s tests in CI Thanks to rust-lang#112701 and `@bjorn3,` it made this much easier. Also cc `@antoyo.` r? `@bjorn3`
…ozkan Run part of `rustc_codegen_gcc`'s tests in CI Thanks to rust-lang#112701 and `@bjorn3,` it made this much easier. Also cc `@antoyo.` r? `@bjorn3`
While currently cg_clif is already built in CI ensuring that it always compiles, sometimes there is a bug in the changes that were made causing tests to fail. This PR is one step towards preventing this.
Part of the test suite is still skipped until vendoring for the projects that are being tested is implemented. I will implement that in a future PR.
Fixes part of https://github.com/bjorn3/rustc_codegen_cranelift/issues/1290
Fixes part of #95518
Required for #81746