Skip to content

update msgpack version from 2.1.5 to 3.3.0 #285

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

agrawalamit2005
Copy link

update msgpack version from 2.1.5 to 3.3.0

@qchateau
Copy link
Collaborator

Hi, thank you for this. Can you please tell me how you updated msgpack ? The diff is too big to review, and the only way I can decently merge this PR while making sure there is no malicious code in there is to do the update myself and check that I end up in the same state as your branch.

I noticed you replaced "msgpack-c" with "clmdep_msgpack-c" so I suppose that code does not comes from the official msgpack repository. Is there a reason for that ? I would really prefer to stick with the official msgpack code.

@agrawalamit2005
Copy link
Author

Hi, thank you for this. Can you please tell me how you updated msgpack ? The diff is too big to review, and the only way I can decently merge this PR while making sure there is no malicious code in there is to do the update myself and check that I end up in the same state as your branch.

I noticed you replaced "msgpack-c" with "clmdep_msgpack-c" so I suppose that code does not comes from the official msgpack repository. Is there a reason for that ? I would really prefer to stick with the official msgpack code.

Sorry for late reply. It is used from official msgpack repository only. For consistency, i had to change it to clmdep_msgpack-c as being done for previous version of msgpack.

@piperoc
Copy link

piperoc commented Jul 21, 2022

Has this been merged? Thanks.

@qchateau
Copy link
Collaborator

No, I cannot realistically review this change, and I can't just trust the author that everything is sound and well-intentionned (no offense, that's just what's expected of a maintainer).

That being said, I'd like to see this merged but I need a way to be confident the upgrade is correct. Can you please provide a script performing the upgrade from the previous version to this one ? This way I can simply review the script, run it myself and check that the diff is identical to this PR.

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.

3 participants