Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Give a better error message when CI download fails #120098

Closed
wants to merge 1 commit into from
Closed

Give a better error message when CI download fails #120098

wants to merge 1 commit into from

Conversation

Teapot4195
Copy link
Contributor

@Teapot4195 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
Copy link
Collaborator

rustbot commented Jan 18, 2024

r? @Mark-Simulacrum

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

@rustbot 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 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
@Teapot4195
Copy link
Contributor Author

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.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@Teapot4195 Teapot4195 force-pushed the issue-118758-fix branch 4 times, most recently from b1dcabf to 05dce7c Compare January 18, 2024 23:59
@Teapot4195 Teapot4195 force-pushed the issue-118758-fix branch 2 times, most recently from 61e204f to 0fdf0a4 Compare January 19, 2024 14:08
Mark-Simulacrum
Mark-Simulacrum reviewed Jan 20, 2024
View reviewed changes
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: "" },
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

src/bootstrap/src/core/download.rs Outdated
}
crate::exit!(1);
}
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
"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");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@Mark-Simulacrum 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
Copy link
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 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
Mark-Simulacrum reviewed Jan 27, 2024
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", "");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

I suspect this is the actual intent.

@Mark-Simulacrum 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
Copy link
Contributor Author

Teapot4195 commented Jan 28, 2024 •
edited
Loading

@rustbot ready

@rustbot 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
Mark-Simulacrum requested changes Feb 4, 2024
View reviewed changes
src/bootstrap/src/core/download.rs Outdated
tempfile: &Path,
url: &str,
help_on_error: &str,
commit: &String,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
commit: &String,
commit: &str,

src/bootstrap/src/core/download.rs Outdated
@@ -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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
}
crate::exit!(1);
}
}

fn check_outdated(commit: &String) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

src/bootstrap/src/core/download.rs Outdated
Command::new("git")
.arg("show")
.arg("-s")
.arg("--format=%ar")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 rustbot removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 4, 2024
@rustbot 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
Copy link
Contributor Author

@rustbot ready

@rustbot 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
Mark-Simulacrum requested changes Feb 18, 2024
View reviewed changes
@rustbot 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
@bors
Copy link
Contributor

bors commented Mar 12, 2024

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

@Teapot4195
Copy link
Contributor Author

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

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bors
Copy link
Contributor

bors commented May 12, 2024

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

@Dylan-DPC Dylan-DPC added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. 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-author Status: This is awaiting some action (such as code changes or more information) from the author. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 5, 2024
@Dylan-DPC
Copy link
Member

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

@JohnCSimon
Copy link
Member

@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 JohnCSimon closed this Oct 6, 2024
@rustbot rustbot added the S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. label Oct 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Noratrieb Noratrieb Noratrieb left review comments

@Mark-Simulacrum Mark-Simulacrum Mark-Simulacrum requested changes

Assignees

@Mark-Simulacrum Mark-Simulacrum

Labels
S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

give a better error when origin/HEAD is vastly out of date
8 participants
@Teapot4195 @rustbot @rust-log-analyzer @bors @Dylan-DPC @JohnCSimon @Mark-Simulacrum @Noratrieb