Skip to content

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

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 2 commits into from
May 30, 2023

Conversation

miss-islington
Copy link
Contributor

@miss-islington miss-islington commented May 26, 2023

(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>
@JelleZijlstra JelleZijlstra self-assigned this May 26, 2023
@JelleZijlstra JelleZijlstra enabled auto-merge (squash) May 26, 2023 12:56
@JelleZijlstra
Copy link
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
Copy link
Member

AlexWaygood commented May 30, 2023

@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 AlexWaygood requested a review from Yhg1s May 30, 2023 10:19
@JelleZijlstra
Copy link
Member

JelleZijlstra commented May 30, 2023

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

@JelleZijlstra
Copy link
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
Copy link
Member

AlexWaygood commented May 30, 2023

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!

@JelleZijlstra
Copy link
Member

Thanks! That diff looks pretty reasonable.

@JelleZijlstra JelleZijlstra merged commit 7899fac into python:3.12 May 30, 2023
@miss-islington miss-islington deleted the backport-ba73473-3.12 branch May 30, 2023 14:33
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants