| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
Sorry, something went wrong.
|
Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead. |
Sorry, something went wrong.
Sorry, something went wrong.
|
@thunder-coding if you do not want to sign the CLA with CLA bot you can fill out the form here: https://www.python.org/psf/contrib/contrib-form/ You could also use the bot and revoke the permissions afterwards. It is open source: https://github.com/psf/clabot |
Sorry, something went wrong.
Essentially a revert of the part where LIBPYTHON is made to use BLDLIBRARY in 7f5e3f0. We can't use BLDLIBRARY as it adds `-L.`, which is not expected behaviour. Part of downstream distribution of Termux porting Python 3.13 to Termux (Android). This is the revised version after proper review and fixes
Yep, that -L. is actually an error. And I can confirm that is an error that should not happen. I've mentioned about this in the commit message. Please check it out. And this should be fixing things properly without causing any breakages or hardcoding LDVERSION/ABIFLAGS |
Sorry, something went wrong.
|
Honestly, I'm tired with this autotools since the beginning of this PR. The first version of the PR along with the 6+ versions I have had pushed (excluding the ones I did not push because I revised them locally), all I have dealt with autotools in a really annoying manner. Any minor change I do in the build process breaks builds for some other target which shouldn't even have a minor impact of the change at all. For every change I have to ./configure, wait for it to finish, and analyze the generated Makefiles to figure out where things went wrong. Ideally I should be able to look at the build configuration, and figure out without having to run anything but this isn't possible as there are some variables which are being used before they are being declared, thanks to the weird substitution technique in autotools of '$(VARIABLE)' I can offer to rewrite the build system in something more modern, more easy to work with for both Python contributors as well as others in the Python ecosystem who want to develop extensions for Python. Many projects have moved over from autotools to more modern build systems like CMake, Meson. I think it's time for CPython as well to make the change now. Git is transitioning from autotools to meson: git/git@904339e I can probably look forward to fixing this issue without moving to a more modern build system, but I do strongly believe that Python needs to make the switch in order to support multiple platforms in the long run, as it'll ensure ease to work with build scripts without someone having to loose their mind and 8 hours trying to figure out how each variable is being assigned. |
Sorry, something went wrong.
|
For discussion on modernising the build system:
I suggest following up there (the first is unlocked). |
Sorry, something went wrong.
|
There's clearly some other side effects to this change; holding off on a review until CI is passing. |
Sorry, something went wrong.
|
@thunder-coding: Sorry, I don't know why the CLA process is taking so long. Meanwhile, please post a comment saying "I license this PR to the Python Software Foundation under the Apache License, Version 2.0". That will at least allow me to look at the code and copy it to a PR of my own if necessary. |
Sorry, something went wrong.
|
Sorry, I couldn't get the time to properly test this PR for builds. Marking this as draft once again. The build system is just painful to deal with, so will be trying to give this another shot with a fresh mind this weekend And for CLA: I license this PR to the Python Software Foundation under the Apache License, Version 2.0 |
Sorry, something went wrong.
|
@thunder-coding please click on the link here to sign the CLA: The PDF form you signed is for organizations. BTW: the "Act on your behalf" permission is poor wording on GitHub's part, which is pretty widely discussed online. The actual "acting on your behalf" is restricted to the other permissions that are listed on the same screen. In the case of our app it is literally just the ability for the app to see which email addresses belong to you and what repositories under the python/ organization you have access to. There's no write access for anything. |
Sorry, something went wrong.
|
@mhsmith please don't suggest people "sign CLAs" by posting comments. This is not a process that was vetted by legal. |
Sorry, something went wrong.
|
Based on the PDF form we received, I manually approved the email address linked to the GitHub username as having signed the CLA. Please proceed with the review, @mhsmith. |
Sorry, something went wrong.
|
Thanks very much Łukasz. The tests are currently failing, so hopefully @thunder-coding will be able to deal with that soon, and then I'll do the review. |
Sorry, something went wrong.
The only places that should have -L. are the command lines for building the extension modules in the stdlib, which are generated by makesetup. So I've added the argument there.
I can't see the commit you were responding to here, but the current approach only affects the LIBPYTHON variable, which will now become a string like -lpython3.16 without any variable references. Since it has to be used in the .pc file where the variables aren't available, I think that's acceptable. The .pc line now looks like this on Android: Libs: -L${libdir} -lpython3.16
|
Sorry, something went wrong.
There was a problem hiding this comment.
I think this is good, and it's clearly passing CI - but I'm not 100% certain that it's correct.
Including -L . as a default argument when building shared modules shouldn't have any side effects as far as I can tell - on platforms that set LIBPYTHON="" because they don't link modules against libpython, it should be a no-op, because there shouldn't be anything else in the directory that could be picked up as link target.
However, it also seems like a change that could have subtle side effects that I'm not aware of.
I'm happy to sign off on this from an Android perspective, but I'd prefer to have someone more familiar with the consequences make a final call on the changes to makesetup.
The line in question was last modified by @picnixz; can you see any reason including -L. as a default here would be an issue?
Sorry, something went wrong.
|
That's a reasonable point, I'll see if I can remove it. |
Sorry, something went wrong.
|
I've updated makesetup to use a new Makefile variable, MODULE_LDFLAGS_SHARED, which has the same value as BLDLIBRARY when extension modules should be linked against libpython, and is otherwise empty. |
Sorry, something went wrong.
There was a problem hiding this comment.
Thanks for those updates - I think this all makes sense now, and should have no impact on any platform other than Android/Cygwin.
Sorry, something went wrong.
|
Thanks @thunder-coding for the PR, and @freakboy3742 for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13, 3.14, 3.15. |
Sorry, something went wrong.
|
GH-155107 is a backport of this pull request to the 3.15 branch. |
Sorry, something went wrong.
|
Sorry, @thunder-coding and @freakboy3742, I could not cleanly backport this to 3.13 due to a conflict. cherry_picker 5918085bb6f4a3a48193cacb9bb99b044d4e0452 3.13 |
Sorry, something went wrong.
|
GH-155108 is a backport of this pull request to the 3.14 branch. |
Sorry, something went wrong.
|
GH-155111 is a backport of this pull request to the 3.13 branch. |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
7f5e3f0 introduced the change to LIBPYTHON where it erraneously depended on BLDLIBRARY which contained -L. -lpython$LDVERSION, this PR addresses that and attempts to fix it
Patch stolen from downstream distribution of Python for Termux (Android): termux/termux-packages#27739 The patch was broken as pointed out by @freakboy3742 , and now will be replaced with this PR's patch.
Preferably needs to be backported to both Python 3.13 as well as Python 3.14, along with the main branch
Fixes #138800