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

Fix binlog corruption with incorrectly serialized blob size. #9057

Closed
wants to merge 1 commit into from

Conversation

filipnavara
Copy link
Member

@filipnavara filipnavara commented Jul 23, 2023

Contributes to fixing xamarin/xamarin-macios#18568

Context

PR #9022 introduced a bug which started incorrectly serializing blobs in .binlog format due to overlooked long vs. int bug (https://github.com/dotnet/msbuild/pull/9022/files#r1271468212).

Changes Made

Added a missing int cast to preserve the original .binlog file format.

Testing

Manually tested that the .binlog is fixed and readable by MSBuildLog viewer after this change.

Notes

@filipnavara filipnavara changed the title Fix binlog corruption which incorrectly serialized blob size. Fix binlog corruption with incorrectly serialized blob size. Jul 23, 2023
Copy link
Member

@dalexsoto dalexsoto left a comment

Choose a reason for hiding this comment

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

❤️

@ladipro
Copy link
Member

ladipro commented Jul 24, 2023

@filipnavara @dalexsoto
I was asked about this PR privately so just wanted to acknowledge that it's being reviewed with priority as it appears to be addressing a recent regression. Thank you!

Copy link
Member

@KirillOsenkov KirillOsenkov left a comment

Choose a reason for hiding this comment

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

Thanks so much for getting to the bottom of it!

@KirillOsenkov
Copy link
Member

As a separate PR (no need to give Filip more work), I recommend that the MSBuild team adds a unit-test that would have caught this regression (unless it's hard for some reason).

@rainersigwald
Copy link
Member

rainersigwald commented Jul 25, 2023

I expect to do it in this PR (but our team can do it--I'm not assigning Filip more work!)--because I expect that Tactics will ask the "do you have a regression test for this?" question when we take this tomorrow.

@MichalPavlik
Copy link
Member

Sorry for the inconvenience. I created PR with test targeting 17.7. In the meantime, you can use ProjectImports=ZipFile parameter to avoid code that causes the corruption. In this case, the archive will be separate file on the disk.

#9065

@filipnavara
Copy link
Member Author

Superseded by #9065. Thanks for swift action.

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.

6 participants