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

Error if submodule fetch fails. #92214

Merged
merged 3 commits into from
Mar 2, 2022
Merged

Error if submodule fetch fails. #92214

merged 3 commits into from
Mar 2, 2022

Conversation

ehuss
Copy link
Contributor

@ehuss ehuss commented Dec 22, 2021

In CI, if fetching a submodule fails, the script would exit successfully. Later parts of the build will fail due to the missing files, but it is a bit confusing, and I think it would be better to error out earlier.

The reason is that in bash, wait without arguments will exit 0 even if a background job exits with an error. The solution here is to wait on each individual job, which will return the exit code of the job.

This was encountered in #92177.

@rust-highfive
Copy link
Collaborator

r? @kennytm

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 22, 2021
@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 23, 2022
@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 27, 2022
@Dylan-DPC
Copy link
Member

r? @Mark-Simulacrum

@Mark-Simulacrum
Copy link
Member

@bors r+ rollup

This makes sense to me, thanks!

@bors
Copy link
Contributor

bors commented Feb 28, 2022

📌 Commit 1233ab7 has been approved by Mark-Simulacrum

@bors 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 Feb 28, 2022
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 28, 2022
@matthiaskrgr
Rollup merge of rust-lang#92214 - ehuss:submodule-bg-exit, r=Mark-Sim…
dc97586 
…ulacrum

Error if submodule fetch fails.

In CI, if fetching a submodule fails, the script would exit successfully. Later parts of the build will fail due to the missing files, but it is a bit confusing, and I think it would be better to error out earlier.

The reason is that in bash, `wait` without arguments will exit 0 even if a background job exits with an error. The solution here is to wait on each individual job, which will return the exit code of the job.

This was encountered in rust-lang#92177.
@matthiaskrgr matthiaskrgr mentioned this pull request Feb 28, 2022
Closed
@matthiaskrgr
Copy link
Member

This is probably causing errors here:
#94443 (comment)
@bors rollup=never

@ehuss
Copy link
Contributor Author

ehuss commented Feb 28, 2022

Oh, sorry about that. I did a bunch of testing when I wrote this and I never saw the tar errors. Those look like legitimate problems with the archives, I'll try to investigate more.

@bors r-

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Feb 28, 2022
@ehuss
Copy link
Contributor Author

ehuss commented Feb 28, 2022

@Mark-Simulacrum I pushed a fix for the extraction error. On Windows, msys will not use symlinks by default. tar will try to copy the file, but that depends on the order of the files in the archive, and will fail if the destination does not exist yet. This adds an env var to tell tar to use normal symlinks, which are enabled on GitHub Actions.

@Mark-Simulacrum
Copy link
Member

@bors r+ rollup=iffy

@bors
Copy link
Contributor

bors commented Feb 28, 2022

📌 Commit 31267e8 has been approved by Mark-Simulacrum

@bors 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 28, 2022
@bors
Copy link
Contributor

bors commented Mar 1, 2022

⌛ Testing commit 31267e8 with merge 9f3945be2c6691c241a804d1d55c6c41cec4c634...

@bors
Copy link
Contributor

bors commented Mar 1, 2022

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Mar 1, 2022
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@ehuss ehuss force-pushed the submodule-bg-exit branch 8 times, most recently from 273d794 to 72508c7 Compare March 1, 2022 18:10
@ehuss
Move submodule checkout before msys2 installation.
43f83bc 
For some reason, `tar` behaves differently in such a way that it does
not create symlinks on Windows correctly, resulting in
`Cannot create symlink to 'ld.gold': No such file or directory`
errors.
@ehuss
Copy link
Contributor Author

ehuss commented Mar 1, 2022

OK, I think this is ready for review again. I had done a bunch of testing on Windows, but didn't have quite the same setup. For some reason, the MSYS2 script causes tar to fail. I resolved that problem by moving the submodule checkout to happen before the MSYS2 script. I'm not sure if that order of steps was intentional, but I don't see a particular problem with that. I also don't entirely know why msys2 was causing problems.

@Mark-Simulacrum
Copy link
Member

@bors r+ rollup=iffy

@bors
Copy link
Contributor

bors commented Mar 2, 2022

📌 Commit 43f83bc has been approved by Mark-Simulacrum

@bors 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 Mar 2, 2022
@bors
Copy link
Contributor

bors commented Mar 2, 2022

⌛ Testing commit 43f83bc with merge 8769f4e...

@bors
Copy link
Contributor

bors commented Mar 2, 2022

☀️ Test successful - checks-actions
Approved by: Mark-Simulacrum
Pushing 8769f4e to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 2, 2022
@bors bors merged commit 8769f4e into rust-lang:master Mar 2, 2022
@rustbot rustbot added this to the 1.61.0 milestone Mar 2, 2022
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (8769f4e): comparison url.

Summary: This benchmark run did not return any relevant results.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

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

@Mark-Simulacrum Mark-Simulacrum

Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
1.61.0
Development

Successfully merging this pull request may close these issues.