Give a better error message when CI download fails #120098

Teapot4195 · 2024-01-18T15:11:34Z

Fixes an issue introduced in 7b5577985df7. Looks like a typo, checked the output from diff and doesn't look like there are any other issues in with the commit.

resolves 118758

rustbot · 2024-01-18T15:11:43Z

r? @Mark-Simulacrum

(rustbot has picked a reviewer for you, use r? to override)

Teapot4195 · 2024-01-18T21:05:47Z

Not exactly as suggested in the original issue, however based on this zulip discussion it seems like putting it under download_http_with_retries is a better choice.

src/bootstrap/src/core/download.rs

Mark-Simulacrum · 2024-01-20T21:27:05Z

src/bootstrap/src/core/download.rs

+            .map(|c| c as char)
+            .collect();
+        let upstream = git::get_rust_lang_rust_remote(
+            &GitConfig { git_repository: "rust-lang/rust.git", nightly_branch: "" },


we should use something like self.config.git_config() or similar, not hardcode these values.

src/bootstrap/src/core/download.rs

Mark-Simulacrum · 2024-01-20T21:29:17Z

src/bootstrap/src/core/download.rs

            }
            crate::exit!(1);
        }
    }

+    fn check_outdated(commit: &String) {
+        let check_outdated_msg = || {
+            if !commit.is_empty() {


It seems surprising to do all of the user.name-based work, and then end up not actually looking at any timestamps anyway because commit.is_empty().

commit.is_empty() is really just a sanity check, it should be very unlikely to hit that branch of the if statement now. Not sure how this should be refactored.

src/bootstrap/src/core/download.rs

Mark-Simulacrum · 2024-01-20T21:31:19Z

src/bootstrap/src/core/download.rs

+                        "NOTE: origin/master is {} days out of date, CI builds disappear in 168 days.",
+                        diff / (24 * 60 * 60)
+                    );
+                    eprintln!("HELP: Consider updating your fork of the rust sources");


fork doesn't sound right here. origin/master should be the rust-lang/rust remote, not a fork.

Original remote is the contributor's fork no? And then usually it would be upstream that is rust-lang/rust.

src/bootstrap/src/core/download.rs

Teapot4195 · 2024-01-20T22:43:29Z

fixed, did some thinking and it doesn't really make sense to check for commits not by the current user in origin/master..HEAD any more, nor does it make sense to be checking if the user's repo is out of date, checking if HEAD is out of date makes much more sense (especially since this is trying to explain why the download of a component failed).

Mark-Simulacrum · 2024-01-27T18:38:49Z

src/bootstrap/src/core/download.rs

+        .unwrap_or("".to_string());
+        match SystemTime::now().duration_since(SystemTime::UNIX_EPOCH) {
+            Ok(n) => {
+                let replaced = last_commit.replace("\n", "");


Suggested change

let replaced = last_commit.replace("\n", "");

let replaced = last_commit.trim();

I suspect this is the actual intent.

src/bootstrap/src/core/download.rs

Teapot4195 · 2024-01-28T16:18:32Z

@rustbot ready

Mark-Simulacrum · 2024-02-04T15:50:32Z

src/bootstrap/src/core/download.rs

+        tempfile: &Path,
+        url: &str,
+        help_on_error: &str,
+        commit: &String,


Suggested change

commit: &String,

commit: &str,

Mark-Simulacrum · 2024-02-04T15:50:38Z

src/bootstrap/src/core/download.rs

@@ -194,7 +195,7 @@ impl Config {
        let _ = try_run(self, patchelf.arg(fname));
    }

-    fn download_file(&self, url: &str, dest_path: &Path, help_on_error: &str) {
+    fn download_file(&self, url: &str, dest_path: &Path, help_on_error: &str, commit: &String) {


Suggested change

fn download_file(&self, url: &str, dest_path: &Path, help_on_error: &str, commit: &String) {

fn download_file(&self, url: &str, dest_path: &Path, help_on_error: &str, commit: &str) {

&String should almost never be in any API.

Mark-Simulacrum · 2024-02-04T15:52:10Z

src/bootstrap/src/core/download.rs

            }
            crate::exit!(1);
        }
    }

+    fn check_outdated(commit: &String) {


Suggested change

fn check_outdated(commit: &String) {

fn check_outdated(commit: &str) {

src/bootstrap/src/core/download.rs

Mark-Simulacrum · 2024-02-04T15:55:31Z

src/bootstrap/src/core/download.rs

+                        Command::new("git")
+                            .arg("show")
+                            .arg("-s")
+                            .arg("--format=%ar")


Why author date here. vs commit date above? Can we add comments indicating what the percent codes mean?

Mark-Simulacrum · 2024-02-04T15:56:38Z

src/bootstrap/src/core/download.rs

+                    .unwrap_or("".to_string());
+                    if build_date.is_empty() {
+                        eprintln!(
+                            "NOTE: tried to download builds for {}, CI builds will be gone in 168 days",


Suggested change

"NOTE: tried to download builds for {}, CI builds will be gone in 168 days",

"NOTE: tried to download builds for {}, CI builds are only retained for 168 days",

Mark-Simulacrum · 2024-02-04T15:57:15Z

src/bootstrap/src/core/download.rs

+                        );
+                    } else {
+                        eprintln!(
+                            "NOTE: tried to download builds for {} (from {}), CI builds will be gone in 168 days",


Suggested change

"NOTE: tried to download builds for {} (from {}), CI builds will be gone in 168 days",

"NOTE: tried to download builds for {} (from {}), CI builds are only retained for 168 days",

Mark-Simulacrum · 2024-02-04T16:01:20Z

src/bootstrap/src/core/download.rs

+                            commit, build_date
+                        );
+                    }
+                    eprintln!("HELP: Consider updating your copy of the rust sources");


This feels like a suggestion we could improve upon, but I guess it's OK for now. It seems like we still have a divergence between rustc components and LLVM components on how we compute the right commit to download, so there's not necessarily single guidance we can give here.

Teapot4195 · 2024-02-05T03:36:41Z

@rustbot ready

Mark-Simulacrum · 2024-02-18T16:24:12Z

src/bootstrap/src/utils/helpers.rs

+        Ok(status) => status,
+        Err(e) => fail(&format!("failed to execute command: {cmd:?}\nERROR: {e}")),
+    };
+    String::from_utf8(output.stdout).unwrap_or("".to_string())


Please just panic or similar if the output isn't utf-8. Returning an empty String is confusing.

Mark-Simulacrum · 2024-02-18T16:24:27Z

src/bootstrap/src/core/download.rs

+                            .arg("--format=%cr") // Commit date in `x days ago` format
+                            .arg(commit),
+                    );
+                    if build_date.is_empty() {


Given below change to panic on non-utf-8, we should remove this branch.

Mark-Simulacrum · 2024-02-18T16:26:23Z

src/bootstrap/src/core/download.rs

@@ -648,7 +695,7 @@ HELP: if trying to compile an old commit of rustc, disable `download-rustc` in c
 download-rustc = false
 ";
        }
-        self.download_file(&format!("{base_url}/{url}"), &tarball, help_on_error);
+        self.download_file(&format!("{base_url}/{url}"), &tarball, help_on_error, &key.to_string());


Please explicitly thread the commit through or rename the argument to commit. Otherwise changing the cache key will break this functionality in a confusing way.

You also shouldn't need to call .to_string() here.

Mark-Simulacrum · 2024-02-18T16:26:32Z

src/bootstrap/src/core/download.rs

+                &format!("{base}/{llvm_sha}/{filename}"),
+                &tarball,
+                help_on_error,
+                &llvm_sha.to_string(),


Suggested change

&llvm_sha.to_string(),

&llvm_sha,

bors · 2024-03-12T19:15:15Z

☔ The latest upstream changes (presumably #122389) made this pull request unmergeable. Please resolve the merge conflicts.

Teapot4195 · 2024-04-22T14:24:51Z

Needed to take a break, but hopefully I understood the changes you wanted to be done.

bors · 2024-05-12T08:57:19Z

☔ The latest upstream changes (presumably #124883) made this pull request unmergeable. Please resolve the merge conflicts.

rustbot assigned Mark-Simulacrum Jan 18, 2024

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 18, 2024

Teapot4195 changed the title ~~download_http_with_retries should only return when Powershell exits with success not failure.~~ Give a better error message when CI download fails Jan 18, 2024

This comment has been minimized.

Sign in to view

Teapot4195 force-pushed the issue-118758-fix branch 4 times, most recently from b1dcabf to 05dce7c Compare January 18, 2024 23:59

Nilstrieb reviewed Jan 19, 2024

View reviewed changes

src/bootstrap/src/core/download.rs Outdated Show resolved Hide resolved

Teapot4195 force-pushed the issue-118758-fix branch 2 times, most recently from 61e204f to 0fdf0a4 Compare January 19, 2024 14:08

Mark-Simulacrum reviewed Jan 20, 2024

View reviewed changes

Mark-Simulacrum 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 20, 2024

Teapot4195 force-pushed the issue-118758-fix branch from 0fdf0a4 to 7358d65 Compare January 20, 2024 22:41

Mark-Simulacrum 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 20, 2024

Mark-Simulacrum reviewed Jan 27, 2024

View reviewed changes

Mark-Simulacrum 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 27, 2024

Teapot4195 force-pushed the issue-118758-fix branch from 7358d65 to 8c8c7cf Compare January 28, 2024 15:30

rustbot 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 28, 2024

Mark-Simulacrum requested changes Feb 4, 2024

View reviewed changes

rustbot removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 4, 2024

rustbot added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Feb 4, 2024

Teapot4195 force-pushed the issue-118758-fix branch from 8c8c7cf to ff84e21 Compare February 4, 2024 18:47

rustbot 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 Feb 5, 2024

Mark-Simulacrum requested changes Feb 18, 2024

View reviewed changes

rustbot 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 Feb 18, 2024

Teapot4195 force-pushed the issue-118758-fix branch from ff84e21 to 6a57629 Compare April 22, 2024 14:24

This comment has been minimized.

Sign in to view

Give a better error message when the CI download fails

c899cbd

Teapot4195 force-pushed the issue-118758-fix branch from 6a57629 to c899cbd Compare April 22, 2024 14:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Give a better error message when CI download fails #120098

Give a better error message when CI download fails #120098

Teapot4195 commented Jan 18, 2024 •

edited

rustbot commented Jan 18, 2024

Teapot4195 commented Jan 18, 2024

This comment has been minimized.

Mark-Simulacrum Jan 20, 2024

Mark-Simulacrum Jan 20, 2024

Teapot4195 Jan 20, 2024

Mark-Simulacrum Jan 20, 2024

Teapot4195 Jan 20, 2024

Teapot4195 commented Jan 20, 2024 •

edited

Mark-Simulacrum Jan 27, 2024

Teapot4195 commented Jan 28, 2024 •

edited

Mark-Simulacrum Feb 4, 2024

Mark-Simulacrum Feb 4, 2024

Mark-Simulacrum Feb 4, 2024

Mark-Simulacrum Feb 4, 2024

Mark-Simulacrum Feb 4, 2024

Mark-Simulacrum Feb 4, 2024

Mark-Simulacrum Feb 4, 2024

Teapot4195 commented Feb 5, 2024

Mark-Simulacrum Feb 18, 2024

Mark-Simulacrum Feb 18, 2024

Mark-Simulacrum Feb 18, 2024

Mark-Simulacrum Feb 18, 2024

bors commented Mar 12, 2024

Teapot4195 commented Apr 22, 2024

This comment has been minimized.

bors commented May 12, 2024

	let replaced = last_commit.replace("\n", "");
	let replaced = last_commit.trim();

	fn download_file(&self, url: &str, dest_path: &Path, help_on_error: &str, commit: &String) {
	fn download_file(&self, url: &str, dest_path: &Path, help_on_error: &str, commit: &str) {

	fn check_outdated(commit: &String) {
	fn check_outdated(commit: &str) {

	"NOTE: tried to download builds for {}, CI builds will be gone in 168 days",
	"NOTE: tried to download builds for {}, CI builds are only retained for 168 days",

	"NOTE: tried to download builds for {} (from {}), CI builds will be gone in 168 days",
	"NOTE: tried to download builds for {} (from {}), CI builds are only retained for 168 days",

Give a better error message when CI download fails #120098

Are you sure you want to change the base?

Give a better error message when CI download fails #120098

Conversation

Teapot4195 commented Jan 18, 2024 • edited

rustbot commented Jan 18, 2024

Teapot4195 commented Jan 18, 2024

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Teapot4195 commented Jan 20, 2024 • edited

Choose a reason for hiding this comment

Teapot4195 commented Jan 28, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Teapot4195 commented Feb 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Mar 12, 2024

Teapot4195 commented Apr 22, 2024

This comment has been minimized.

bors commented May 12, 2024

Teapot4195 commented Jan 18, 2024 •

edited

Teapot4195 commented Jan 20, 2024 •

edited

Teapot4195 commented Jan 28, 2024 •

edited