Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

python3: introduce libpython3 with ABI_VERSION flag #14237

Merged
merged 1 commit into from May 1, 2021

Conversation

commodo
Copy link
Contributor

@commodo commodo commented Dec 15, 2020

Maintainer: me, @jefferyto
Compile tested: x86 openwrt/openwrt@36e35b8
Run tested: x86 openwrt/openwrt@36e35b8

Related to discussion:
#14060

Every once in a while a version bump will occur that requires an ABI
change. Example: Python 3.8 to 3.9. When this happens some Python packages
would need to be rebuilt. In setups where everything gets rebuilt, this
isn't a problem.

It's usually a bigger problem when needing to upgrade something via
opkg.
To accommodate for this, we add a libpython with it's own ABI_VERSION
flag. If this ABI_VERSION changes, then this should propagate forward.

Signed-off-by: Alexandru Ardelean ardeleanalex@gmail.com

@BKPepe
Copy link
Member

BKPepe commented Dec 15, 2020

Hmm, it looks good. What do you think, @Cynerd?

In Turris, we are using this patch, so we know which version of Python 3 is used across OpenWrt versions (19.07 and master):
https://gitlab.nic.cz/turris/turris-build/-/blob/v5.1.4/patches/packages/to-upstream/0011-python3-Make-python-version-part-of-release-number.patch

@jefferyto
Copy link
Member

Looking into ABI_VERSION a bit more, it looks like its main (only?) use is for shared libraries; the wiki has a surprisingly detailed description of what it does.

The ABI_VERSION value is set inside the define Package/... section, e.g. libopenssl, and it changes the package name from libopenssl to libopenssl1.1. Other packages can continue to list libopenssl as a dependency and the build system will figure out the "real" dependency and when things need to be rebuilt.

A direct parallel for Python, and in a way a direct fix for the uwsgi issue, would be to split libpython into a separate package and set ABI_VERSION for it. Then packages that have a build dependency on libpython can depend on this library package and will be rebuilt when the ABI changes.

I'm not sure if there would be any other purpose for splitting libpython into a separate package though.

Thoughts?

@commodo
Copy link
Contributor Author

commodo commented Dec 16, 2020

oh; i did not dig into ABI_VERSION too close;
thanks for doing that;

so, there is some precedence in ruby using ABI_VERSION for libruby;

the thing that makes Python special [especially on OpenWrt] is the fact that packages are shipped as byte-compiled;
so that in itself is an ABI_VERSION change;

now, whether we tie that libpython the ABI_VERSION or not, makes little difference I guess;
because I doubt there would be people that would install just libpython and not have the python interpreter;
though, who knows, there are always weird people using Python on OpenWrt;

so, i guess adding a libpython sub-package is not difficult, so might as well do it now;
it is the fully correct approach and may matter later;

i'll work on a v2

@commodo commodo changed the title python3: introduce ABI_VERSION flag python3: introduce libpython3 with ABI_VERSION flag Feb 24, 2021
@commodo
Copy link
Contributor Author

commodo commented Feb 24, 2021

so, it took me a while to re-spin this; ¯_(ツ)_/¯

Changelog v1 -> v2:

  • split libpython3 ; this seems to be the more appropriate way to use ABI_VERSION (after being suggested by @jefferyto and looking around);

@BKPepe
Copy link
Member

BKPepe commented Apr 10, 2021

@jefferyto ? :)

@commodo
Copy link
Contributor Author

commodo commented Apr 12, 2021

@jefferyto has been a bit inactive in the last few weeks;

that is fine; probably busy with other stuff;
i'd say that we wait 1-2 weeks as-is and merge it [if there is still silence];

@commodo
Copy link
Contributor Author

commodo commented Apr 28, 2021

@jefferyto
ping on this real quick;
maybe you have some time to look over this;

Copy link
Member

@jefferyto jefferyto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apologies again for disappearing / continuing to be missing 🙇

# Adding the lib-dynload folder (even just empty) suppresses 2 warnings when starting Python
$(INSTALL_DIR) $(1)/usr/lib/python$(PYTHON3_VERSION)/lib-dynload/
$(INSTALL_DIR) $(1)/usr/bin
$(LN) python$(PYTHON3_VERSION) $(1)/usr/bin/python3
$(LN) python$(PYTHON3_VERSION) $(1)/usr/bin/python
$(CP) $(PKG_INSTALL_DIR)/usr/lib/libpython$(PYTHON3_VERSION).so* $(1)/usr/lib/
# This depends on being called before filespec is processed
$(SED) 's|$(TARGET_AR)|ar|g;s|$(TARGET_CROSS)readelf|readelf|g;s|$(TARGET_CC)|gcc|g;s|$(TARGET_CXX)|g++|g' \
$(PKG_INSTALL_DIR)/usr/lib/python$(PYTHON3_VERSION)/_sysconfigdata.py
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these lines modifying _sysconfigdata.py can be kept/moved into Py3Package/python3-base/install since the file is installed by python3-base.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'm feeling _sysconfigdata.py may be a little better suited under libpython
but i won't object much to moving it;
we can move it;

initially i was thinking of cases where some users may want to install just libpython for some weird reasons;
that would be a sort of "python-devel" [or libpython-dev] let's say;
getting to "python-devel" is a long way and may require more elements; so whether _sysconfigdata.py stays in python3-base or libpython3, doesn't make much difference;

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_sysconfigdata.py was always in python3-base (it is included in PYTHON3_BASE_LIB_FILES). This sed line only replaced references to $(TARGET_CC)/$(TARGET_CXX)/etc with gcc/g++/...

Related to discussion:
  openwrt#14060

Every once in a while a version bump will occur that requires an ABI
change. Example: Python 3.8 to 3.9. When this happens some Python packages
would need to be rebuilt. In setups where everything gets rebuilt, this
isn't a problem.

It's usually a bigger problem when needing to upgrade something via
opkg.
To accommodate for this, we add a libpython with it's own ABI_VERSION
flag. If this ABI_VERSION changes, then this should propagate forward.

Signed-off-by: Alexandru Ardelean <ardeleanalex@gmail.com>
@commodo
Copy link
Contributor Author

commodo commented Apr 28, 2021

Changelog v2 -> v3:

  • moved _sysconfigdata.py under python3-base package ;
  • rebased on newer master

Copy link
Member

@jefferyto jefferyto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@BKPepe BKPepe merged commit b11a9f2 into openwrt:master May 1, 2021
@commodo commodo deleted the python-abi-version branch May 4, 2021 08:41
@jefferyto
Copy link
Member

I just noticed something (apologies for missing it earlier): because the package is called libpython3 and ABI_VERSION is appended to the package name, the package is now libpython33.9, which looks a bit... silly.

@commodo would you be okay if I opened a PR to set ABI_VERSION to .$(PYTHON_VERSION_MINOR) (assuming setting it to a string beginning with "." will not cause issues) so that the package is called libpython3.9?

@commodo
Copy link
Contributor Author

commodo commented Jun 14, 2021

@commodo would you be okay if I opened a PR to set ABI_VERSION to .$(PYTHON_VERSION_MINOR) (assuming setting it to a string beginning with "." will not cause issues) so that the package is called libpython3.9?

fine from me;
go for it :)

@jow-
Copy link
Contributor

jow- commented Jun 14, 2021

the package is now libpython33.9, which looks a bit... silly.

This is actually an upstream OpenWrt bug which was introduced due to improper refactoring. For packages ending with a digit, a dash should be inserted automatically, so the resulting package should be libpython3-3.9. I'll hopefully be able to look into it this week.

would you be okay if I opened a PR to set ABI_VERSION to .$(PYTHON_VERSION_MINOR) (assuming setting it to a string beginning with "." will not cause issues)

Please don't do that.

@commodo
Copy link
Contributor Author

commodo commented Jun 14, 2021

the package is now libpython33.9, which looks a bit... silly.

This is actually an upstream OpenWrt bug which was introduced due to improper refactoring. For packages ending with a digit, a dash should be inserted automatically, so the resulting package should be libpython3-3.9. I'll hopefully be able to look into it this week.

would you be okay if I opened a PR to set ABI_VERSION to .$(PYTHON_VERSION_MINOR) (assuming setting it to a string beginning with "." will not cause issues)

Please don't do that.

thanks for the intervention :)

@jefferyto
Copy link
Member

This is actually an upstream OpenWrt bug which was introduced due to improper refactoring. For packages ending with a digit, a dash should be inserted automatically, so the resulting package should be libpython3-3.9. I'll hopefully be able to look into it this week.

Thanks @jow- 🙏

jow- added a commit to openwrt/openwrt that referenced this pull request Jun 14, 2021
Ensure that ABI suffixes are separated with a dash from the package name if
the name happens to end with a digit. This implementation detail got lost
during the recent refactoring of the ABI_VERSION handling in buildroot.

Ref: openwrt/packages#14237 (comment)
Fixes: c921650 ("build: drop ABI version from metadata")
Signed-off-by: Jo-Philipp Wich <jo@mein.io>
Vladdrako pushed a commit to Vladdrako/openwrt that referenced this pull request Jun 15, 2021
Ensure that ABI suffixes are separated with a dash from the package name if
the name happens to end with a digit. This implementation detail got lost
during the recent refactoring of the ABI_VERSION handling in buildroot.

Ref: openwrt/packages#14237 (comment)
Fixes: c921650 ("build: drop ABI version from metadata")
Signed-off-by: Jo-Philipp Wich <jo@mein.io>
jollaman999 pushed a commit to jollaman999/openwrt that referenced this pull request Jun 17, 2021
Ensure that ABI suffixes are separated with a dash from the package name if
the name happens to end with a digit. This implementation detail got lost
during the recent refactoring of the ABI_VERSION handling in buildroot.

Ref: openwrt/packages#14237 (comment)
Fixes: c921650382 ("build: drop ABI version from metadata")
Signed-off-by: Jo-Philipp Wich <jo@mein.io>
jow- added a commit to openwrt/openwrt that referenced this pull request Jul 11, 2021
Ensure that ABI suffixes are separated with a dash from the package name if
the name happens to end with a digit. This implementation detail got lost
during the recent refactoring of the ABI_VERSION handling in buildroot.

Ref: openwrt/packages#14237 (comment)
Fixes: c921650 ("build: drop ABI version from metadata")
Signed-off-by: Jo-Philipp Wich <jo@mein.io>
(cherry picked from commit f6a03bf)
maurerr pushed a commit to maurerr/openwrt that referenced this pull request Sep 1, 2021
Ensure that ABI suffixes are separated with a dash from the package name if
the name happens to end with a digit. This implementation detail got lost
during the recent refactoring of the ABI_VERSION handling in buildroot.

Ref: openwrt/packages#14237 (comment)
Fixes: c921650 ("build: drop ABI version from metadata")
Signed-off-by: Jo-Philipp Wich <jo@mein.io>
ArtelMike pushed a commit to ArtelMike/openwrt-1 that referenced this pull request Jan 31, 2023
Ensure that ABI suffixes are separated with a dash from the package name if
the name happens to end with a digit. This implementation detail got lost
during the recent refactoring of the ABI_VERSION handling in buildroot.

Ref: openwrt/packages#14237 (comment)
Fixes: c0f2488 ("build: drop ABI version from metadata")
Signed-off-by: Jo-Philipp Wich <jo@mein.io>
(cherry picked from commit 9167734)
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.

None yet

4 participants