Revisit fix_is_ci_llvm_available logic #107234

Rattenkrieg · 2023-01-23T18:05:33Z

Fixes #107225
Now supported_platforms has a knowledge whether llvm asserts artifacts are available for particular host triple.

@jyn514 @albertlarsan68 PTAL

rustbot · 2023-01-23T18:05:43Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @ozkanonur (or someone else) soon.

Please see the contribution instructions for more information.

Rattenkrieg · 2023-01-23T18:07:20Z

src/bootstrap/native.rs

+        ("i686-pc-windows-gnu", false),
+        ("i686-pc-windows-msvc", false),
+        ("i686-unknown-linux-gnu", false),
+        ("x86_64-unknown-linux-gnu", true),


First five are false according to now removed

if (triple == "aarch64-unknown-linux-gnu" || triple.contains("i686")) && asserts { return false }

Rattenkrieg · 2023-01-23T18:08:06Z

src/bootstrap/native.rs

+        ("x86_64-unknown-freebsd", false),
+        ("x86_64-unknown-illumos", false),
+        ("x86_64-unknown-linux-musl", false),
+        ("x86_64-unknown-netbsd", false),


No "llvm with asserts" artifacts are published for tier 2 with host tools.

src/bootstrap/config/tests.rs

jyn514 · 2023-01-23T18:36:54Z

r? @albertlarsan68 - feel free to ask if you want advice on the review :)

albertlarsan68

Just a nit, r=me when done and CI passes

albertlarsan68 · 2023-01-24T07:27:05Z

src/bootstrap/config/tests.rs

@@ -19,6 +19,8 @@ fn download_ci_llvm() {
    assert_eq!(parse_llvm(""), if_available);
    assert_eq!(parse_llvm("rust.channel = \"dev\""), if_available);
    assert!(!parse_llvm("rust.channel = \"stable\""));
+    assert!(parse_llvm("llvm.assertions = true \r\n build.build = \"x86_64-unknown-linux-gnu\" \r\n llvm.download-ci-llvm = \"if-available\""));
+    assert_eq!(parse_llvm("llvm.assertions = true \r\n build.build = \"aarch64-apple-darwin\" \r\n llvm.download-ci-llvm = \"if-available\""), false);


Suggested change

assert_eq!(parse_llvm("llvm.assertions = true \r\n build.build = \"aarch64-apple-darwin\" \r\n llvm.download-ci-llvm = \"if-available\""), false);

assert!(!parse_llvm("llvm.assertions = true \r\n build.build = \"aarch64-apple-darwin\" \r\n llvm.download-ci-llvm = \"if-available\""));

Asserting for false should be done in this way rather than asserting equality with false.

Got it! I'll get back to these tests later today, since there are formatting issues and adding build.build didn't help.

Rattenkrieg · 2023-01-24T19:02:46Z

src/bootstrap/config.rs

@@ -965,6 +965,9 @@ impl Config {
        config.changelog_seen = toml.changelog_seen;

        let build = toml.build.unwrap_or_default();
+        if let Some(file_build) = build.build {
+            config.build = TargetSelection::from_user(&file_build);
+        };


NB build triple wasn't read from result of get_toml. I don't know whether it was skipped deliberately or just simply forgotten. I'm leaning towards the latter, since build.host and build.target are read below from the same toml.

probably accidental; https://github.com/rust-lang/rust/blob/8adbe6e436d7ae03dbb0e82729745e1e514eb508/src/bootstrap/config.rs#L815 gets it right because python sets it for https://github.com/rust-lang/rust/blob/8adbe6e436d7ae03dbb0e82729745e1e514eb508/src/bootstrap/build.rs#L4-L6 in https://github.com/rust-lang/rust/blob/8adbe6e436d7ae03dbb0e82729745e1e514eb508/src/bootstrap/bootstrap.py#L788-L797, so likely no one ever noticed.

I think it shouldn't ever matter in practice; even when we stop using python (#94829) the shell scripts should still respect build.build in config.toml, but changing it to make things testable seems good 👍

Rattenkrieg · 2023-01-27T05:29:44Z

@albertlarsan68 is there anything else left for me to address?

jyn514 · 2023-01-27T06:10:13Z

@Rattenkrieg Albert is on vacation until next week

Rattenkrieg · 2023-01-27T06:11:28Z

@Rattenkrieg Albert is on vacation until next week

Got it, sorry, no rush then.

albertlarsan68 · 2023-01-27T06:19:39Z

I think this is good. Can you squash the commits please?

Tip: post a comment containing @rustbot ready when you want another review, it puts the PR back into the to-do list of the reviewer.

Rattenkrieg · 2023-01-27T07:21:57Z

@rustbot ready

albertlarsan68 · 2023-01-27T09:33:25Z

Thanks!
@bors r+ rollup

bors · 2023-01-27T09:33:27Z

📌 Commit 9ef8407 has been approved by albertlarsan68

It is now in the queue for this repository.

…vm_available, r=albertlarsan68 Revisit fix_is_ci_llvm_available logic Fixes rust-lang#107225 Now `supported_platforms` has a knowledge whether llvm asserts artifacts are available for particular host triple. `@jyn514` `@albertlarsan68` PTAL

Rollup of 9 pull requests Successful merges: - rust-lang#106806 (Replace format flags u32 by enums and bools.) - rust-lang#107194 (Remove dependency on slice_internals feature in rustc_ast) - rust-lang#107234 (Revisit fix_is_ci_llvm_available logic) - rust-lang#107316 (Update snap from `1.0.1` to `1.1.0`) - rust-lang#107321 (solver comments + remove `TyCtxt::evaluate_goal`) - rust-lang#107332 (Fix wording from `rustbuild` to `bootstrap`) - rust-lang#107347 (reduce rightward-drift) - rust-lang#107352 (compiler: Fix E0587 explanation) - rust-lang#107357 (Fix infinite loop in rustdoc get_all_import_attributes function) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

rustbot assigned onur-ozkan Jan 23, 2023

rustbot added 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 23, 2023

Rattenkrieg commented Jan 23, 2023

View reviewed changes

src/bootstrap/config/tests.rs Outdated Show resolved Hide resolved

rustbot assigned albertlarsan68 and unassigned onur-ozkan Jan 23, 2023

This comment has been minimized.

Sign in to view

albertlarsan68 approved these changes Jan 24, 2023

View reviewed changes

albertlarsan68 added 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 24, 2023

Rattenkrieg commented Jan 24, 2023

View reviewed changes

jyn514 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 27, 2023

Revisit fix_is_ci_llvm_available logic; read build triple from toml

9ef8407

Rattenkrieg force-pushed the bootstrap-fix-is_ci_llvm_available branch from 8adbe6e to 9ef8407 Compare January 27, 2023 07:19

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-review Status: Awaiting review from the assignee but also interested parties. labels Jan 27, 2023

Dylan-DPC mentioned this pull request Jan 27, 2023

Rollup of 6 pull requests #107356

Closed

JohnTitor mentioned this pull request Jan 27, 2023

Rollup of 9 pull requests #107372

Merged

bors merged commit 04dfde4 into rust-lang:master Jan 27, 2023

rustbot added this to the 1.69.0 milestone Jan 27, 2023

albertlarsan68 mentioned this pull request Jan 31, 2023

Confusing error message with download-ci-llvm on old checkout #107506

Closed

jyn514 mentioned this pull request Feb 28, 2023

Update LLVM submodule #107879

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Revisit fix_is_ci_llvm_available logic #107234

Revisit fix_is_ci_llvm_available logic #107234

Rattenkrieg commented Jan 23, 2023

rustbot commented Jan 23, 2023

Rattenkrieg Jan 23, 2023

Rattenkrieg Jan 23, 2023

jyn514 commented Jan 23, 2023

This comment has been minimized.

albertlarsan68 left a comment

albertlarsan68 Jan 24, 2023

Rattenkrieg Jan 24, 2023

Rattenkrieg Jan 24, 2023

jyn514 Jan 24, 2023 •

edited

Loading

Rattenkrieg commented Jan 27, 2023

jyn514 commented Jan 27, 2023

Rattenkrieg commented Jan 27, 2023

albertlarsan68 commented Jan 27, 2023

Rattenkrieg commented Jan 27, 2023

albertlarsan68 commented Jan 27, 2023

bors commented Jan 27, 2023

	assert_eq!(parse_llvm("llvm.assertions = true \r\n build.build = \"aarch64-apple-darwin\" \r\n llvm.download-ci-llvm = \"if-available\""), false);
	assert!(!parse_llvm("llvm.assertions = true \r\n build.build = \"aarch64-apple-darwin\" \r\n llvm.download-ci-llvm = \"if-available\""));

Revisit fix_is_ci_llvm_available logic #107234

Revisit fix_is_ci_llvm_available logic #107234

Conversation

Rattenkrieg commented Jan 23, 2023

rustbot commented Jan 23, 2023

Rattenkrieg Jan 23, 2023

Choose a reason for hiding this comment

Rattenkrieg Jan 23, 2023

Choose a reason for hiding this comment

jyn514 commented Jan 23, 2023

This comment has been minimized.

albertlarsan68 left a comment

Choose a reason for hiding this comment

albertlarsan68 Jan 24, 2023

Choose a reason for hiding this comment

Rattenkrieg Jan 24, 2023

Choose a reason for hiding this comment

Rattenkrieg Jan 24, 2023

Choose a reason for hiding this comment

jyn514 Jan 24, 2023 • edited Loading

Choose a reason for hiding this comment

Rattenkrieg commented Jan 27, 2023

jyn514 commented Jan 27, 2023

Rattenkrieg commented Jan 27, 2023

albertlarsan68 commented Jan 27, 2023

Rattenkrieg commented Jan 27, 2023

albertlarsan68 commented Jan 27, 2023

bors commented Jan 27, 2023

jyn514 Jan 24, 2023 •

edited

Loading