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.

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

[Merged by Bors] - Fix CI #4675

Closed
wants to merge 1 commit into from
Closed

Conversation

BoxyUwU
Copy link
Member

@BoxyUwU BoxyUwU commented May 6, 2022

No description provided.

@github-actions github-actions bot added the S-Needs-Triage This issue needs to be labelled label May 6, 2022
@mockersf
Copy link
Member

mockersf commented May 6, 2022

is it due to a regression in rust nightly, or is it something that we actually need to fix in Bevy?

@BoxyUwU
Copy link
Member Author

BoxyUwU commented May 6, 2022

regression but i'd be kind of surprised if it was fixed since it doesnt seem like a clear cut "bug"
..plus I'd like one of my other PRs to get merged and not have to wait ages for if they do fix it

Copy link
Contributor

@superdump superdump left a comment

Choose a reason for hiding this comment

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

I don't fully understand the nitty gritty details, just the high-level concept that the rust compiler has changed something in nightly that affects the way it is able to prove lifetime constraints for various types/traits that caused this regression, and that this PR changes the lifetime annotations in such a way that the rust compiler can verify their validity. Thanks to @BoxyUwU for taking the time on Discord to discuss with and explain to me what this is about.

I also think it is important to unblock CI. We discussed whether the problem should be fixed upstream (yes?) and whether it's likely to happen soon (no?). We discussed alternatives to unblock CI such as make rust nightly builds failing not block CI (would make rust nightly users sad, and miri jobs identifying undefined behaviour would have to also be made non-blocking, which is also bad) and didn't come up with anything that feels better than merging this PR.

As such, I approve this.

@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events A-Build-System Related to build systems or continuous integration P-Critical This must be fixed immediately or contributors or users will be severely impacted and removed S-Needs-Triage This issue needs to be labelled labels May 6, 2022
@mockersf
Copy link
Member

mockersf commented May 6, 2022

bors r+

bors bot pushed a commit that referenced this pull request May 6, 2022
@bors bors bot changed the title Fix CI [Merged by Bors] - Fix CI May 6, 2022
@bors bors bot closed this May 6, 2022
@mockersf
Copy link
Member

mockersf commented May 6, 2022

Discussion about this in the rust-lang repo: rust-lang/rust#89352 (comment)
For now, waiting for a crater run to see the impact

robtfm pushed a commit to robtfm/bevy that referenced this pull request May 10, 2022
exjam pushed a commit to exjam/bevy that referenced this pull request May 22, 2022
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Build-System Related to build systems or continuous integration A-ECS Entities, components, systems, and events P-Critical This must be fixed immediately or contributors or users will be severely impacted
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants