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

Bump protobuf version to 4.24.4 #4553

Merged
merged 21 commits into from Feb 28, 2024
Merged

Bump protobuf version to 4.24.4 #4553

merged 21 commits into from Feb 28, 2024

Conversation

bartbroere
Copy link
Contributor

@bartbroere bartbroere commented Feb 23, 2024

This PR bumps protobuf to 4.25.3 4.24.4.
The 4.25.x release needs a bit more work.

#4558 contains more extensive testing for this package.

Copy link
Member

@hoodmane hoodmane left a comment

Choose a reason for hiding this comment

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

Thanks @bartbroere. Nice to increase test coverage a bit.

@hoodmane
Copy link
Member

By the way, I have been thinking about what we can do to improve the ergonomics of the test system. Let me know if you have opinions.

@ryanking13
Copy link
Member

It looks like there is a signature mismatch. Could you take a look if possible? Probably this document could help you: https://pyodide.org/en/stable/development/debugging.html

@bartbroere
Copy link
Contributor Author

It looks like there is a signature mismatch. Could you take a look if possible? Probably this document could help you: https://pyodide.org/en/stable/development/debugging.html

Thanks for that resource. I'll be looking into this.

@bartbroere bartbroere changed the title Bump protobuf version to 4.25.3 Bump protobuf version to 4.24.4 Feb 24, 2024
Copy link
Member

@hoodmane hoodmane left a comment

Choose a reason for hiding this comment

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

Patch is confusing but it does make the added tests pass. Thanks @bartbroere. Do you understand why:

  1. it compiles successfully without the patch
  2. it works correctly in native Python (I assume) but not in Pyodide?

I'd appreciate any explanation you can give.

Copy link
Member

Choose a reason for hiding this comment

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

I'm a bit confused about how this compiles successfully both before and after this patch. Is upb_MessageDef_MiniTable a macro that casts the argument?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On my end and in the CI it didn't build before this patch. (Note that I backed off a bit and first bumped to 4.24.x before even starting on 4.25.x support).

https://app.circleci.com/pipelines/github/pyodide/pyodide/6887/workflows/01b9a7e4-a06d-4b17-a990-bd8bd57c28e2/jobs/93955/parallel-runs/0/steps/0-104?invite=true#step-104-32873_80

Before the patch I got this error on the three lines addressed in the patch: error: incompatible integer to pointer conversion. This led me to think that the argument was being converted to a pointer implicitly, and decided to make it explicit.

I'll add a test to see if I broke the deepcopy functionality of protobuf messages by just turning the argument into a pointer.

Copy link
Member

@hoodmane hoodmane Feb 26, 2024

Choose a reason for hiding this comment

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

Well this error comes from the compile flag:

-Werror=int-conversion

if appropriate you could pass -Wno-int-conversion in cflags to turn it off.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, that seems to do the trick. The newly added tests indeed showed the wrong memory being accessed with the patch.

@bartbroere
Copy link
Contributor Author

Patch is confusing but it does make the added tests pass. Thanks @bartbroere. Do you understand why:

  1. it compiles successfully without the patch
  2. it works correctly in native Python (I assume) but not in Pyodide?

I'd appreciate any explanation you can give.

My initial guess was that there's a difference between how typical C compilers and the Emscripten C compile handle implicit conversion to a pointer, but your theory that a macro might be responsible for this could also be true. I'll look into both theories.

Just to be sure I also added a test to see if this breaks something in a segmentation fault kind of way.

@hoodmane
Copy link
Member

+1 for turn off flag and add a test that uses these functions.

@hoodmane
Copy link
Member

@bartbroere should I merge this?

@bartbroere
Copy link
Contributor Author

@bartbroere should I merge this?

Yes, please. And thanks for the help!

@hoodmane hoodmane merged commit 7dd7030 into pyodide:main Feb 28, 2024
41 checks passed
@hoodmane
Copy link
Member

Thanks @bartbroere!

1 similar comment
@ryanking13
Copy link
Member

ryanking13 commented Feb 29, 2024

Thanks @bartbroere!

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.

None yet

3 participants