Skip to content

Fix binlog corruption with incorrectly serialized blob size.#9057

Closed
filipnavara wants to merge 1 commit intodotnet:mainfrom
filipnavara:blobsizefix
Closed

Fix binlog corruption with incorrectly serialized blob size.#9057
filipnavara wants to merge 1 commit intodotnet:mainfrom
filipnavara:blobsizefix

Conversation

@filipnavara
Copy link
Copy Markdown
Member

@filipnavara filipnavara commented Jul 23, 2023 •
edited
Loading

Contributes to fixing dotnet/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
dalexsoto
dalexsoto approved these changes Jul 24, 2023
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
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!

KirillOsenkov
KirillOsenkov approved these changes Jul 25, 2023
View reviewed changes
Copy link
Copy Markdown
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 KirillOsenkov mentioned this pull request Jul 25, 2023
Merged
@KirillOsenkov
Copy link
Copy Markdown
Member

KirillOsenkov commented Jul 25, 2023

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
Copy Markdown
Member

rainersigwald commented Jul 25, 2023 •
edited
Loading

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
Copy Markdown
Member

MichalPavlik commented Jul 25, 2023

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
Copy Markdown
Member Author

filipnavara commented Jul 26, 2023

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

Reviewers

@dalexsoto dalexsoto dalexsoto approved these changes

@KirillOsenkov KirillOsenkov KirillOsenkov approved these changes

Assignees

@MichalPavlik MichalPavlik

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@filipnavara @ladipro @KirillOsenkov @rainersigwald @MichalPavlik @dalexsoto