-
Notifications
You must be signed in to change notification settings - Fork 21
Fix/msgpack unity #1324
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
base: feature/support-msgpack
Are you sure you want to change the base?
Fix/msgpack unity #1324
Conversation
IL2CPP compatible - Updated msgpack tests accordingly - Updated deps.cake with proper dep paths
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
0b79913 to
d6c2e04
Compare
tigran-sargsyan-w
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review
Overall this is a solid step towards making the Unity + MsgPack setup more maintainable and IL2CPP‑friendly.
What looks good
- New
DepsandLicenseHelperhelpers keep build / license logic out of the main Cake scripts and are easy to reuse. - Explicit dependency checks with clear error messages should make missing DLL issues much easier to diagnose in CI and on dev machines.
- Unity edit/play mode tests being wired into the MsgPack setup helps ensure the Unity path stays in sync with the core library.
Questions / suggestions
- In
DownloadLicenseContentyou create a newHttpClientper call and block on.Result. For build scripts this is usually fine, but it might be safer to either reuse a singleHttpClientinstance or at least document that the build requires network access to GitHub and these URLs. - When a dependency is missing or a license download fails, the thrown
Exceptioncurrently stops the build (which is good), but it could be helpful to hint which script/target was running (e.g. include more context in the message) to simplify debugging. - Since this PR is still marked as draft and there are TODOs about updating the Newtonsoft DLL and .NET Framework license, it might be worth adding a short comment in the Cake tasks to make it obvious that those parts are intentionally left for a follow‑up commit.
Once the remaining TODO items are addressed, this looks good to me. 👍
TODO