Give a better error message when CI download fails#120098

Closed

Teapot4195 wants to merge 1 commit intorust-lang:masterfrom

Teapot4195:issue-118758-fix

Contributor

Teapot4195 commented Jan 18, 2024 •

edited

Loading

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 assigned Mark-Simulacrum

Collaborator

rustbot commented Jan 18, 2024

r? @Mark-Simulacrum

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

rustbot added S-waiting-on-review T-bootstrap labels

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

Contributor Author

Teapot4195 commented Jan 18, 2024

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.

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

Noratrieb reviewed

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

View reviewed changes

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

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

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

src/bootstrap/src/core/download.rs Outdated

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

Member

Mark-Simulacrum Jan 20, 2024

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

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

src/bootstrap/src/core/download.rs Outdated

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

Member

Mark-Simulacrum Jan 20, 2024

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().

Contributor Author

Teapot4195 Jan 20, 2024

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 Outdated Show resolved Hide resolved

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

src/bootstrap/src/core/download.rs Outdated

+                                      "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");

Member

Mark-Simulacrum Jan 20, 2024

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

Contributor Author

Teapot4195 Jan 20, 2024

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 Outdated Show resolved Hide resolved

Mark-Simulacrum added S-waiting-on-author and removed S-waiting-on-review labels

Teapot4195 force-pushed the issue-118758-fix branch from 0fdf0a4 to 7358d65 Compare

January 20, 2024 22:41

Contributor Author

Teapot4195 commented Jan 20, 2024 •

edited

Loading

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 added S-waiting-on-review and removed S-waiting-on-author labels

Mark-Simulacrum reviewed

View reviewed changes

src/bootstrap/src/core/download.rs Outdated

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

Member

Mark-Simulacrum Jan 27, 2024

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 Show resolved Hide resolved

Mark-Simulacrum added S-waiting-on-author and removed S-waiting-on-review labels

Teapot4195 force-pushed the issue-118758-fix branch from 7358d65 to 8c8c7cf Compare

January 28, 2024 15:30

Contributor Author

Teapot4195 commented Jan 28, 2024 •

edited

Loading

rustbot added S-waiting-on-review and removed S-waiting-on-author labels

Mark-Simulacrum requested changes

View reviewed changes

src/bootstrap/src/core/download.rs Outdated

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

Member

Mark-Simulacrum Feb 4, 2024

Suggested change

      
                    commit: &String,
          
                    commit: &str,

src/bootstrap/src/core/download.rs Outdated

                   }
-                  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) {

Member

Mark-Simulacrum Feb 4, 2024

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.

src/bootstrap/src/core/download.rs Outdated

                       }
                   }
+                  fn check_outdated(commit: &String) {

Member

Mark-Simulacrum Feb 4, 2024

Suggested change

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

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

src/bootstrap/src/core/download.rs Outdated

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

Member

Mark-Simulacrum Feb 4, 2024

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

src/bootstrap/src/core/download.rs Outdated

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

Member

Mark-Simulacrum Feb 4, 2024

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",

src/bootstrap/src/core/download.rs Outdated

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

Member

Mark-Simulacrum Feb 4, 2024

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",

src/bootstrap/src/core/download.rs

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

Member

Mark-Simulacrum Feb 4, 2024

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.

rustbot removed the S-waiting-on-review label

rustbot added the S-waiting-on-author label

Teapot4195 force-pushed the issue-118758-fix branch from 8c8c7cf to ff84e21 Compare

February 4, 2024 18:47

Contributor Author

Teapot4195 commented Feb 5, 2024

rustbot added S-waiting-on-review and removed S-waiting-on-author labels

Mark-Simulacrum requested changes

View reviewed changes

src/bootstrap/src/utils/helpers.rs Outdated Show resolved Hide resolved

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

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

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

rustbot added S-waiting-on-author and removed S-waiting-on-review labels

Collaborator

bors commented Mar 12, 2024

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

Teapot4195 force-pushed the issue-118758-fix branch from ff84e21 to 6a57629 Compare

April 22, 2024 14:24

Contributor Author

Teapot4195 commented Apr 22, 2024

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

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

Collaborator

bors commented May 12, 2024

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

Dylan-DPC added S-waiting-on-review S-waiting-on-author and removed S-waiting-on-author S-waiting-on-review labels

Member

Dylan-DPC commented Aug 6, 2024

@Teapot4195 this is still waiting on the review suggestions, if you can update those or mark them as resolved we can move this forward

Member

JohnCSimon commented Oct 6, 2024

@Teapot4195
I'm closing this due to inactivity because this hasn't been touched in a few months. Please reopen when you are ready to continue with this. Thank you for your contribution.

@rustbot label: +S-inactive

JohnCSimon closed this

rustbot added the S-inactive label

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-inactive S-waiting-on-author T-bootstrap