FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

[3.12] gh-104799: Move location of type_params AST fields (GH-104828) by miss-islington · Pull Request #104974 · python/cpython · GitHub

/ cpython Public

[3.12] gh-104799: Move location of type_params AST fields (GH-104828) - #104974

Merged
JelleZijlstra merged 2 commits into
python:3.12from
miss-islington:backport-ba73473-3.12
May 30, 2023
Merged

[3.12] gh-104799: Move location of type_params AST fields (GH-104828)#104974
JelleZijlstra merged 2 commits into
python:3.12from
miss-islington:backport-ba73473-3.12

Conversation

miss-islington commented May 26, 2023
edited by github-actions Bot
Loading

Copy link
Copy Markdown
Contributor

(cherry picked from commit ba73473)

Co-authored-by: Jelle Zijlstra jelle.zijlstra@gmail.com
Co-authored-by: Alex Waygood Alex.Waygood@Gmail.com


📚 Documentation preview 📚: https://cpython-previews--104974.org.readthedocs.build/

…4828)

(cherry picked from commit ba73473)

Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>

Copy link
Copy Markdown
Member

Seems like this changes the ABI! cc @Yhg1s

I don't know whether the ABI change is relevant in practice, because external users shouldn't be constructing these internal AST nodes anyway. So maybe we should decide this is a false positive. If we think this is in fact a real issue that could affect users, I would vote to revert the change on main too and leave #104799 unfixed, as it's a relatively minor issue and the AST doesn't guarantee full backwards compatibility anyway.

AlexWaygood commented May 30, 2023
edited
Loading

Copy link
Copy Markdown
Member

@Yhg1s, thoughts on this, as release manager? My preference would be to get this merged into 3.12 ASAP, as it minimizes the backwards-compatibility breakage between 3.11 and 3.12 that comes from the PEP-695 implementation. But if the failing ABI check makes that impossible, then we should revert the change that's been made on main (which this PR is backporting) ASAP.

AlexWaygood requested a review from Yhg1s May 30, 2023 10:19

JelleZijlstra commented May 30, 2023
edited by AlexWaygood
Loading

Copy link
Copy Markdown
Member

@Yhg1s said on Discord that the ABI change is fine (thanks!). I'm running the script to regenerate the ABI locally now.

Copy link
Copy Markdown
Member

Unfortunately I couldn't get the script to work. On Mac something is wrong with libmpdec

gcc -I./Modules/_decimal/libmpdec -DCONFIG_64=1 -DANSI=1 -DHAVE_UINT128_T=1 -fno-strict-overflow -DNDEBUG -g -O3 -Wall -g3 -O0 -g3 -O0  -std=c11 -Werror=implicit-function-declaration -fvisibility=hidden  -I./Include/internal  -I. -I./Include   -fPIC -fPIC -c ./Modules/_decimal/_decimal.c -o Modules/_decimal/_decimal.o
gcc -shared      Modules/_decimal/_decimal.o -lm Modules/_decimal/libmpdec/libmpdec.a  -o Modules/_decimal.cpython-312-aarch64-linux-gnu.so
/usr/bin/ld: Modules/_decimal/_decimal.o: in function `context_getprec':
/src/./Modules/_decimal/_decimal.c:740: undefined reference to `mpd_getprec'
/usr/bin/ld: Modules/_decimal/_decimal.o: in function `context_getemax':
/src/./Modules/_decimal/_decimal.c:741: undefined reference to `mpd_getemax'

On an AWS Ubuntu instance I get error="docker-credential-ecr-login can only be used with Amazon Elastic Container Registry.".

AlexWaygood commented May 30, 2023
edited
Loading

Copy link
Copy Markdown
Member

I used the workflow Steve is demo-ing over at #105088 to regen the ABI via GitHub Actions on my CPython fork. I think I did it right. At least the GitHub Actions check on this PR is now passing!

Copy link
Copy Markdown
Member

Thanks! That diff looks pretty reasonable.

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
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.

4 participants


Back | FazBrowse Home | New Git URL