Re: [Rpm-maint] [rpm-software-management/rpm] WIP: Use Python Stable ABI for the bindings (PR #2674)
Merged #2674 into master. -- Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/2674#event-10561805696 You are receiving this because you are subscribed to this thread. Message ID: ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] WIP: Use Python Stable ABI for the bindings (PR #2674)
Nice, thanks! -- Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/2674#issuecomment-1748632819 You are receiving this because you are subscribed to this thread. Message ID: ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] WIP: Use Python Stable ABI for the bindings (PR #2674)
I did that, and I also updated the version in the `INSTALL` file. I didn't test this build config with all the Python versions, and I won't get to that next week, but will do it the week of Oct 16. I find any issues, I can open a fixup PR. -- Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/2674#issuecomment-1748450429 You are receiving this because you are subscribed to this thread. Message ID: ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] WIP: Use Python Stable ABI for the bindings (PR #2674)
@encukou pushed 2 commits. 9c96b7882d7db454818c8ed8a9953e91b7b9ac15 Python stable ABI: Define Py_TPFLAGS_IMMUTABLETYPE as no-op if missing efdd3ede6c11938d0252f9125fabf6326b349614 Python Stable ABI: Build for Stable ABI, require Python 3.7+ -- View it on GitHub: https://github.com/rpm-software-management/rpm/pull/2674/files/8343d71e5043713a38daf3436a7e489134421acf..efdd3ede6c11938d0252f9125fabf6326b349614 You are receiving this because you are subscribed to this thread. Message ID: ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] WIP: Use Python Stable ABI for the bindings (PR #2674)
I'd prefer the "Fix comment style" commit to be merged into the previous commit that introduces the comment, but other than that looks fine to me. Unless you have something else you're planning to work on here, just flag ready for review and I'll merge, we already did the review part here. -- Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/2674#issuecomment-1746264256 You are receiving this because you are subscribed to this thread. Message ID: ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] WIP: Use Python Stable ABI for the bindings (PR #2674)
@encukou pushed 2 commits. 6394b1d583a3dae612c49baa6ad28b8b555d410b Fix comment style 8343d71e5043713a38daf3436a7e489134421acf Python Stable ABI: Build for Stable ABI, require Python 3.7+ -- View it on GitHub: https://github.com/rpm-software-management/rpm/pull/2674/files/1f41d7e4513b0f5c3dfbd7f1f19e60774bd02b15..8343d71e5043713a38daf3436a7e489134421acf You are receiving this because you are subscribed to this thread. Message ID: ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] WIP: Use Python Stable ABI for the bindings (PR #2674)
The cmake nits aside, I briefly skimmed through this and seems like a lot of fairly mechanical transformations (even saw a mention of coccinelle in there), and nothing in the "OMG we can't do that" department. So just trim out the extra complications from the cmake department and I'll be happy to merge. -- Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/2674#issuecomment-1744871771 You are receiving this because you are subscribed to this thread. Message ID: ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] WIP: Use Python Stable ABI for the bindings (PR #2674)
I think that's still unnecessarily complicated :sweat_smile: In a case like this using cmake's integration doesn't help because it's only *more* code and fu to support. I don't really want to see a single option related to this because those are just extra baggage that can break and need testing. Just use the manual cmake thing always, unconditionally, and update the Python requirement in INSTALL to 3.7. We'll switch to the cmake integrated Development.SABIModule version when we some day bump our cmake version to one that supports it. Apologies for not clearly stating this earlier. -- Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/2674#issuecomment-1744863054 You are receiving this because you are subscribed to this thread. Message ID: ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] WIP: Use Python Stable ABI for the bindings (PR #2674)
Would this work? - CMake 3.26+ default: - Build for stable ABI using CMake's mechanism - Needs Python 3.7+ - Older CMake, and builds with `-DENABLE_PYTHON_SABI=OFF`: - Needs Python 3.2+ - On Python 3.7+, build for stable ABI "manually" by defining (I haven't tested, I'm still not sure what should be done) -- Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/2674#issuecomment-1744841420 You are receiving this because you are subscribed to this thread. Message ID: ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] WIP: Use Python Stable ABI for the bindings (PR #2674)
@encukou pushed 1 commit. 1f41d7e4513b0f5c3dfbd7f1f19e60774bd02b15 Select Python 3.7+ stable ABI "manually" if CMake doesn't support it -- View it on GitHub: https://github.com/rpm-software-management/rpm/pull/2674/files/ccc3af9c97ba635f8ef66aad4333c3ad0f617bcc..1f41d7e4513b0f5c3dfbd7f1f19e60774bd02b15 You are receiving this because you are subscribed to this thread. Message ID: ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] WIP: Use Python Stable ABI for the bindings (PR #2674)
@encukou pushed 2 commits. 69db01e2c2a6321cd3df9b8b60c5fb9f407a039b Fix comment style ccc3af9c97ba635f8ef66aad4333c3ad0f617bcc Select Python 3.7+ stable ABI "manually" if CMake doesn't support it -- View it on GitHub: https://github.com/rpm-software-management/rpm/pull/2674/files/1da65d26b64c56a22e393edf9a43c2e9d803cb83..ccc3af9c97ba635f8ef66aad4333c3ad0f617bcc You are receiving this because you are subscribed to this thread. Message ID: ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] WIP: Use Python Stable ABI for the bindings (PR #2674)
> Requiring Python >= 3.7 is not a problem, I'm just curious as to what makes > that particular version special. [`PySlice_GetIndicesEx`](https://docs.python.org/3/c-api/slice.html#c.PySlice_GetIndicesEx) uses a deprecated implementation (on all Python versions) if ABI compatibility with 3.6 is needed. That implementation is only unsafe in rather exotic edge cases ([indexing by an object that has side-effects on conversion to `int`, or another thread changing a sequence size](https://github.com/python/cpython/issues/72054)), but it does emit a compile-time warning. If y'all can live with the warning, I think (but haven't checked), that would allow ABI compatibility with Python 3.4+. But, IMO it's better to use version-specific ABI for such old Python versions. > Requiring Python >= 3.7 is not a problem, I'm just curious as to what makes > that particular version special. Cmake >= 3.26 is too new for us to require > though, but this shouldn't be hard to do manually. Looking at the cmake's > FindPython module, something like target_compile_definitions(_rpm PRIVATE > Py_LIMITED_API=x) (where x is the desired version obviosly) for in > python/CMakeLists.txt I suppose. There's something about linking to a > specific library too, but according to > https://docs.python.org/3/c-api/stable.html that's a Windows-only thing. Here's what's different for stable ABI builds: - Compiling with `Py_LIMITED_API=0x0307`, so only “stable” API is allowed. - Naming the `.so` appropriately. Currently `cmake` outputs `_rpm.so`, which doesn't need to change -- any Python will load it. In the future CMake *might* switch to `_rpm.cpython-311-x86_64-linux-gnu.so`/`_rpm.abi3.so`, but I bet it'll use a `cmake_policy` for that kind of change, so there'll be plenty of time to adjust. - On Windows, linking with a different Python library if appropriate So, yeah, adding `target_compile_definitions(_rpm PRIVATE Py_LIMITED_API=Py_LIMITED_API=0x0307)` will do the trick. But it might be better to use CMake's `Development.SABIModule` if it's available? -- Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/2674#issuecomment-1743057129 You are receiving this because you are subscribed to this thread. Message ID: ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] WIP: Use Python Stable ABI for the bindings (PR #2674)
See my earlier comment, I don't WANT it to fall back to current behavior because then you never really know whether a patch is SABI compliant or not because your setup *may* depend on these factors. -- Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/2674#issuecomment-1740906819 You are receiving this because you are subscribed to this thread. Message ID: ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] WIP: Use Python Stable ABI for the bindings (PR #2674)
As it stands, we gracefully fall back to the current behavior anyway, so I don't think the CMake stuff is a reason to hold up this PR. -- Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/2674#issuecomment-1740870818 You are receiving this because you are subscribed to this thread. Message ID: ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] WIP: Use Python Stable ABI for the bindings (PR #2674)
Nah. It ain't that hard really. -- Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/2674#issuecomment-1740851046 You are receiving this because you are subscribed to this thread. Message ID: ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] WIP: Use Python Stable ABI for the bindings (PR #2674)
Well, I guess then we should hope for [my CentOS Stream MR](https://gitlab.com/redhat/centos-stream/rpms/cmake/-/merge_requests/11) to be merged? -- Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/2674#issuecomment-1740838362 You are receiving this because you are subscribed to this thread. Message ID: ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] WIP: Use Python Stable ABI for the bindings (PR #2674)
I did look at the FindPython module, and to me that looks just the kind of gibberish we left autotools behind for. No thanks. I'm fine importing it as long as it's somebody elses headache but that's the end of that. -- Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/2674#issuecomment-1740826454 You are receiving this because you are subscribed to this thread. Message ID: ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] WIP: Use Python Stable ABI for the bindings (PR #2674)
@pmatilai I would just backport the FindPython module and conditionally use it if CMake is too old. -- Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/2674#issuecomment-1740820797 You are receiving this because you are subscribed to this thread. Message ID: ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] WIP: Use Python Stable ABI for the bindings (PR #2674)
I think, if we go for the stable ABI we should then stick to it too, meaning simply cut off support for versions where not available. Requiring Python >= 3.7 is not a problem, I'm just curious as to what makes that particular version special. Cmake >= 3.26 is too new for us to require though, but this shouldn't be hard to do manually. Looking at the cmake's FindPython module, something like `target_compile_definitions(_rpm PRIVATE Py_LIMITED_API=x)` in python/CMakeLists.txt I suppose. There's something about linking to a specific library too, but according to https://docs.python.org/3/c-api/stable.html that's a Windows-only thing. -- Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/2674#issuecomment-1740817395 You are receiving this because you are subscribed to this thread. Message ID: ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] WIP: Use Python Stable ABI for the bindings (PR #2674)
> With CMake 3.26+, stable ABI will be used by default. This is fine, we can work in subsequent PRs to handle it internally for older CMake versions. -- Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/2674#issuecomment-1738117094 You are receiving this because you are subscribed to this thread. Message ID: ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
