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

Disable buggy LTO #320

Merged
merged 11 commits into from
Nov 25, 2023
Merged

Conversation

Veetaha
Copy link
Contributor

@Veetaha Veetaha commented Nov 24, 2023

Fixes marker_rustc_driver crashing on windows with exit code: 0xc0000005, STATUS_ACCESS_VIOLATION. Reported in #318 (comment).

I disabled LTO, in release build and it fixes the problem.

I extended the CI to perform a really simple release smoke test both for the current release and for the previous release (in github action test CI job).

Here, for example, you may see that the bug reproduces in master on the changed CI

With the disabled LTO release-smoke-test passes

Btw. @xFrednet could you add the release-smoke-test jobs to required ones please?

@Veetaha Veetaha changed the title Disable LTO Disable buggy LTO Nov 24, 2023
@Veetaha Veetaha marked this pull request as ready for review November 24, 2023 23:36
@xFrednet
Copy link
Member

xFrednet commented Nov 25, 2023

Thank you for finding the issue so quickly and the new CI test ❤️

Do we want to release v0.4.2 with this patch? In that case, could you update the changelog entry?

could you add the release-smoke-test jobs to required ones please?

Yes of course! I'll add it, once this PR is done with the merge queue.

@Veetaha
Copy link
Contributor Author

Veetaha commented Nov 25, 2023

Yes, let's do a 0.4.2. Btw. each patch release breaks our CI workflow, which is pinned to a specific cargo-marker version in a docker image, and it complains that marker_api 0.4.1 isn't compatible with 0.4.0 (which it should be, that problem is tracked in #225). Though, it's not a big deal for me to update the version and make a new image, still a good reminder for me that #307 needs to be implemented 😅


I think you'll need to merge this one bypassing the CI status checks because github actions test on windows is going to be red until 0.4.2 is released

@Veetaha
Copy link
Contributor Author

Veetaha commented Nov 25, 2023

I have no idea what happened there, so I just squashed cargo release into a single command, since single invocation works.

@Veetaha
Copy link
Contributor Author

Veetaha commented Nov 25, 2023

There was also this hang on driver ARM build. I also have no idea what happened there. It could be a bug in rustc or qemu. I just reduced the timeout to 60 minutes. For me the build works fine and finishes within ~11 minutes.

Copy link
Member

@xFrednet xFrednet left a comment

Choose a reason for hiding this comment

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

LGTM, that's another bug down!

Let's get the v0.4.2 party started :D

@xFrednet xFrednet merged commit bb0928e into rust-marker:master Nov 25, 2023
24 of 26 checks passed
@xFrednet
Copy link
Member

And the smoke tests have been added to the CI requirements :D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants