gh-115398: Increment PyExpat_CAPI_MAGIC for SetReparseDeferralEnabled addition by gpshead · Pull Request #116301 · python/cpython (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation3 Commits2 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

gpshead

Member

@gpshead gpshead commented

Mar 4, 2024

•

edited by bedevere-appbot

Loading

@gpshead

@gpshead

@gpshead gpshead marked this pull request as ready for review

March 4, 2024 10:14

This was referenced

Mar 4, 2024

woodruffw pushed a commit to woodruffw-forks/cpython that referenced this pull request

Mar 4, 2024

@gpshead @woodruffw

…nabled addition (pythonGH-116301)

This is a followup to git commit 6a95676 from Github PR python#115623.

@encukou

This looks unnecessary: There's an extra size field that's also checked. See #115623 (comment) .
@gpshead, were you aware of that discussion?

hartwork added a commit to hartwork/cpython that referenced this pull request

Mar 6, 2024

@hartwork

@hartwork

It's also hurting, so I have created pull request #116411 now to revert the bump.

gpshead pushed a commit that referenced this pull request

Mar 6, 2024

@hartwork

Revert "gh-115398: Increment PyExpat_CAPI_MAGIC for SetReparseDeferralEnabled addition (GH-116301)"

This reverts part of commit eda2963. Why? this comment buried in an earlier code review explains:

I checked again how that value is used in practice, it's here:

https://github.com/python/cpython/blob/0c80da4c14d904a367968955544dd6ae58c8101c/Modules/_elementtree.c#L4363-L4372

Based on that code my understanding is that loading bigger structs from the future is considered okay unless PyExpat_CAPI_MAGIC differs, which implies that (1) magic needs to stay the same to support loading the future from the past and (2) that PyExpat_CAPI_MAGIC should only ever change for changes that do not increase size (but keep it constant).

To summarize, that supports your argument. I checked branches 3.8, 3.9, 3.10, 3.11, 3.12 now and they all have the same comparison code there so reverting that magic string bump will support seamless backporting.

@gpshead

I hadn't seen the earlier review comment, I just looked at the history of the magic and saw it was bumped up to 1.1 during a bugfix release 6+ years ago. I trust y'all's analysis. revert merged.

@gpshead gpshead deleted the fixup/gh115398/details branch

March 6, 2024 17:58

adorilson pushed a commit to adorilson/cpython that referenced this pull request

Mar 25, 2024

@gpshead @adorilson

…nabled addition (pythonGH-116301)

This is a followup to git commit 6a95676 from Github PR python#115623.

adorilson pushed a commit to adorilson/cpython that referenced this pull request

Mar 25, 2024

@hartwork @adorilson

…16411)

Revert "pythongh-115398: Increment PyExpat_CAPI_MAGIC for SetReparseDeferralEnabled addition (pythonGH-116301)"

This reverts part of commit eda2963. Why? this comment buried in an earlier code review explains:

I checked again how that value is used in practice, it's here:

https://github.com/python/cpython/blob/0c80da4c14d904a367968955544dd6ae58c8101c/Modules/_elementtree.c#L4363-L4372

Based on that code my understanding is that loading bigger structs from the future is considered okay unless PyExpat_CAPI_MAGIC differs, which implies that (1) magic needs to stay the same to support loading the future from the past and (2) that PyExpat_CAPI_MAGIC should only ever change for changes that do not increase size (but keep it constant).

To summarize, that supports your argument. I checked branches 3.8, 3.9, 3.10, 3.11, 3.12 now and they all have the same comparison code there so reverting that magic string bump will support seamless backporting.

diegorusso pushed a commit to diegorusso/cpython that referenced this pull request

Apr 17, 2024

@gpshead @diegorusso

…nabled addition (pythonGH-116301)

This is a followup to git commit 6a95676 from Github PR python#115623.

diegorusso pushed a commit to diegorusso/cpython that referenced this pull request

Apr 17, 2024

@hartwork @diegorusso

…16411)

Revert "pythongh-115398: Increment PyExpat_CAPI_MAGIC for SetReparseDeferralEnabled addition (pythonGH-116301)"

This reverts part of commit eda2963. Why? this comment buried in an earlier code review explains:

I checked again how that value is used in practice, it's here:

https://github.com/python/cpython/blob/0c80da4c14d904a367968955544dd6ae58c8101c/Modules/_elementtree.c#L4363-L4372

Based on that code my understanding is that loading bigger structs from the future is considered okay unless PyExpat_CAPI_MAGIC differs, which implies that (1) magic needs to stay the same to support loading the future from the past and (2) that PyExpat_CAPI_MAGIC should only ever change for changes that do not increase size (but keep it constant).

To summarize, that supports your argument. I checked branches 3.8, 3.9, 3.10, 3.11, 3.12 now and they all have the same comparison code there so reverting that magic string bump will support seamless backporting.