Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Our records indicate we have not received your CLA. For legal reasons we need you to sign this before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

If you have recently signed the CLA, please wait at least one business day
before our records are updated.

You can check yourself to see if the CLA has been received.

Thanks again for your contribution, we look forward to reviewing it!

Right now the build is failing on Windows because we haven't tagged the libffi sources. I'd rather not do that until we have it all working, so for now, can you change the get-externals references to simply libffi without the 3.3? That should pull it from the branch tip, and simplify things if we need to patch it any more.

Once we're ready to merge, we can tag the libffi sources and update the reference here.

There are some configured headers missing from cpython-source-deps
Also need a matching change python.props for the change in get_externals.bat

The CI build is now blocked by a missing fficonfig.h.
I added this PR to cpython-source-deps to add the missing header files: python/cpython-source-deps#7

I moved the configured headers to modules/_ctypes/libffi_msvc so that they are not part of the libffi snapshot but generated from the snapshot, and manually edited. I think this is all working now

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

And if you don't make the requested changes, you will be poked with soft cushions!

Moved configured files to cpython-source-deps as requested, and fixed quotes.
python/cpython-source-deps#8 will need to be merged before CI will pass now
Re-ran all of the build and test_ctypes tests to confirm that things look good locally

python/cpython-source-deps#8 will need to be merged before CI will pass now

It's merged. You can close/reopen to trigger CI again.

@zooba Does libffi-3.3.0-rc0-r1 sound like a reasonable tag for python/cpython-source-deps@8ba2b2c?

@zware Works for me. I'd also like to have the update process documented somewhere (possibly just as a prepare_libffi.bat file, if it's simpler). Hopefully we'll get to 3.3.0 before Python 3.8 releases.

Ok, I've gone ahead and pushed the tag.

@zware Works for me. I'd also like to have the update process documented somewhere (possibly just as a prepare_libffi.bat file, if it's simpler). Hopefully we'll get to 3.3.0 before Python 3.8 releases.

The configured file was hand-edited to merge changes from the original PR and generated configured header. I don't know how to document this.

The sysv_intel.S and ffi.c changes for x86 changes were merged into libffi libffi/libffi#468. However I still need to figure out why autoconf is not working for msvc x86 when building libffi with cygwin.

Once that works we could potentially check in configured files for each CPU type based on the output of the autoconf build step instead of having one merged fficonfig.h

Does the documentation step need to be done before these changes can be merged? Or can there be a seperate issue to follow up on?

@paulmon Let's put rough notes about the update process into the readme.txt file.

Do you have logs for the Cygwin failure? Technically that isn't required for us to merge this, but if we can figure it out relatively easily then I'd prefer to figure it out.

Which readme.txt? PCBuild/readme.txt or cpython-sources-dep/master/readme.rst? or some other file?
The steps for x64 were to follow the appveyor.yml steps from libffi manually. I had to add x86 support and I tried to update the autoconf file but it still says the build type is not supported. The PR for the Win32 changes was merged into master, and there is an open issue I created that I plan to follow up on: libffi/libffi#469. I can make time to look at that today, and update the readme

@zooba @zware

While I was working on the libffi autoconf for arm32 I learned how to build the libffi-7.dll from libffi sources.
I just pushed the changes needed to use libffi-7.dll instead of including the libffi sources in _ctypes.
These changes pass all of the test_ctypes tests for x86/x64 both debug and release.

The changes to _ctypes.vcxproj will require pre-building libffi*.dll (see prepare_libffi.bat) and populating libffi-bin-3.3.0-rc0-r1 before they _ctypes will build, which reminds me I need to update get_externals.bat to get the prebuilt libffi*.dll still.

Also the win32 changes got merged into libffi/master so you should be able to take a new snapshot and build libffi-bin-3.3.0-rc0-r1 from unpatched sources. arm32 changes for libffi are in a PR that is still waiting for attention.

If VS 2019 is installed then build.bat finds the new msbuild.exe which causes a build error. Setting the MSBUILD environment variable back to the VS 2017 msbuild.exe is a better workaround than changing the project. I have reported the build problem and am following up on it: https://developercommunity.visualstudio.com/content/problem/487337/writelinestofile-doesnt-write-new-lines.html

@paulmon I got a successful build of libffi - it's on cpython-bin-deps. Please switch your version name to just libffi for now and I'll tag it once we know the build works with the rest of your changes.

Make sure you sync your code too - I pushed some batch file changes to your branch.

@zooba Looks good.
After I sync'd I changed get_externals.bat to only get the cpython-bin-deps libffi by default.
I also updated python.props.
I built x86/x64, release and debug, and ran test_ctypes on all of them again.

@zware In case you want to review again, feel free (not sure exactly what's changed since your last one). Otherwise I'll try and get through this today and get it merged.

The Ubuntu failure on Azure Pipelines has been happening randomly all day. I wonder if a test was updated that's changing permissions on the temp folder? (They run in random order)

Like the change for PC/layout, there's a tools/msi/lib file (lib_files.wxs?) that will need a reference to the new DLL. It should already have the SSL references in it.

Otherwise, I think the rest looks fine.

The Pipelines failure is due to an unrelated Linux issue, so ignoring that check to merge.

Thanks again @paulmon for seeing this through! It's been a goal for a few years now, it's nice to see it finally happen :)