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

python38: initial version #4413

Merged
merged 6 commits into from
Feb 17, 2021
Merged

python38: initial version #4413

merged 6 commits into from
Feb 17, 2021

Conversation

smaarn
Copy link
Contributor

@smaarn smaarn commented Jan 29, 2021

Motivation: New python 3.8 package containing python version 3.8 (since it's now the oldest "stable" version)
Linked issues: closes #4284

Checklist

  • Build rule all-supported completed successfully
  • Package upgrade completed successfully
  • New installation of package completed successfully

@smaarn smaarn mentioned this pull request Jan 29, 2021
34 tasks
spk/python38/Makefile Outdated Show resolved Hide resolved
cross/python38/Makefile Outdated Show resolved Hide resolved
@hgy59
Copy link
Contributor

hgy59 commented Jan 29, 2021

🥇 for the dedicated python38 package name. This allows rolling updates of python 3.7 dependent packages.
The same does synology with different php 7 versions.

spk/python38/Makefile Outdated Show resolved Hide resolved
cross/python38/Makefile Outdated Show resolved Hide resolved
spk/python38/Makefile Outdated Show resolved Hide resolved
@hgy59
Copy link
Contributor

hgy59 commented Jan 31, 2021

I am awainting for this, as I have a homeassistent update ready that depends on Python 3.8.

@smaarn smaarn requested a review from hgy59 January 31, 2021 20:18
Copy link
Contributor

@hgy59 hgy59 left a comment

Choose a reason for hiding this comment

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

LGTM

@Safihre
Copy link
Contributor

Safihre commented Feb 1, 2021

I am confused, why a seperate package for 3.8?! I know some packages required <3.6 but why would a package run on 3.7 but not on 3.8?
Now when I update SABnzbd I would have to force users to download an additional python? Probably messing things up with the "original" python3?

@smaarn
Copy link
Contributor Author

smaarn commented Feb 1, 2021

I am confused, why a seperate package for 3.8?! I know some packages required <3.6 but why would a package run on 3.7 but not on 3.8?

Now when I update SABnzbd I would have to force users to download an additional python? Probably messing things up with the "original" python3?

There may be issues with other spks depending on python: wheels are not compiled for "any" python version. We could upgrade the current python version but then it would be in "big bang" mode, meaning that users would need to update all depending spks.
To be honest I don't have any strong opinion but at least, this way, we get to have spks tested progressively and by interested parties.

@Safihre
Copy link
Contributor

Safihre commented Feb 1, 2021

Alright, I understand the big bang problem indeed. That's a good reason to split them up.
When just need to keep track that Python packages get updated and not just left on the old Python.
But how do you avoid the dual Python problem? So each package uses the correct one?
I know for SAB we just use /usr/bin/python3. Just like many other packages..

@Forge36
Copy link

Forge36 commented Feb 1, 2021

Should there be a project to track python 3.8 migration completion? (Python 38 full version?)

@hgy59
Copy link
Contributor

hgy59 commented Feb 2, 2021

I know for SAB we just use /usr/bin/python3. Just like many other packages..

Indeed, me must care that python38 does not create a link /usr/bin/python3 and dependent packages must no rely on this.
I think most of the python3 dependent packages do not use such a link, but add /var/packages/python3/target/bin to the package specific path (at least startable services).

EDIT:
Sorry I meant: do not create a link /usr/local/python3.
Any package that links to /usr/bin/python3 uses the python3 package of synology and not the SynoCommunity one.
BTW: the creation of links like /usr/local/{package-name} => /var/packages/{package-name}/target or /usr/local/{package-name} => /volume1/@appstore/{package-name} will not work with DSM >= 7.0.

@rs1gg
Copy link

rs1gg commented Feb 10, 2021

As far as I understand the state of this PR, the community agrees to have separate packages for python minor versions, because it allows to separate the update lifecycles for different applications (some applications need >=3.8, not all applications can go from 3.7 to 3.8 at the same point in time).

For the symlinks (as hgy59 points out) I think it would be ok if:

  • /usr/local/bin/python3 still points to 3.7 (from the python3 package) in order to not break existing dependencies
  • /usr/local/bin/python38 points to current 3.8.x (from the python38 package)

which is already what happens right now.

Applications that explicitly need 3.8.x (such as current homeassistant core) can use /usr/local/bin/python38. I already have a first prototype based on smaarn:feat/python38 with HA core 2021.2.2 compiling and running on XPEnology (arch-bromolow).

What does it take for this request to be merged? I don't want to rush, I just want to understand the procedures.

@Safihre
Copy link
Contributor

Safihre commented Feb 11, 2021

@rs1gg you seem exactly right.
@smaarn ready to merge? Does it need more testing?

@smaarn
Copy link
Contributor Author

smaarn commented Feb 11, 2021

@rs1gg you seem exactly right.

@smaarn ready to merge? Does it need more testing?

All good on my end in terms of tests. Ready to merge on my end.

- hard code python version in PLIST and remove custom PLIST_TRANSFORM in cross/python38/Makefile
- remove obsolete PLIST_TRANSFORM in spk/python38/Makefile
- remove obsolete links to target/usr/bin
- use SynoCommunity as package maintainer
- define SPK_VERS with full version
@hgy59 hgy59 merged commit 682f4d8 into SynoCommunity:master Feb 17, 2021
@smaarn smaarn deleted the feat/python38 branch February 17, 2021 20:13
@hgy59 hgy59 added the status/published Published and activated (may take up to 48h until visible in DSM package manager) label Feb 17, 2021
# Byte-compile in background
PYTHON_SHORT_VER=$(${SYNOPKG_PKGDEST}/bin/python3 -c 'import sys; print("{0}.{1}".format(*sys.version_info[:2]))')
${SYNOPKG_PKGDEST}/bin/python3 -m compileall -q -f ${SYNOPKG_PKGDEST}/lib/python${PYTHON_SHORT_VER} >> ${INST_LOG} &
${SYNOPKG_PKGDEST}/bin/python3 -OO -m compileall -q -f ${SYNOPKG_PKGDEST}/lib/python${PYTHON_SHORT_VER} >> ${INST_LOG} &
Copy link
Contributor

Choose a reason for hiding this comment

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

@hgy59 I was testing this package on my NAS which is ancient, and I saw that the compileall is run twice.
Do you know why?
Or maybe @smaarn knows?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To be honest this was a brutal copy paste without any questioning on the purpose.
I'll be checking for next package release (a 3.8.8 version was recently released).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems that both steps are generating different kind of output and are dedicated to generating python compiled files which will be used depending on the "optimization" requested at runtime (see https://docs.python.org/3/using/cmdline.html#cmdoption-o ).
I'm guessing both run could actually be useful, but since I'm no python expert I should rather keep it quiet here ;)

Copy link
Contributor

Choose a reason for hiding this comment

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

The background compilation was always done twice, even in the python 2.7 package of December 2015

    # Byte-compile in background
    ${INSTALL_DIR}/bin/python -m compileall -q -f ${INSTALL_DIR}/lib/python2.7 > /dev/null &
    ${INSTALL_DIR}/bin/python -OO -m compileall -q -f ${INSTALL_DIR}/lib/python2.7 > /dev/null &

So I suppose this is done by intention.
The first line creates *.pyc files and the second one *.pyo. But I don't know whether this has any benefit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Safihre @hgy59 As a matter of fact, if we really wanted to keep some kind of logic we should even re-run a third time it with a single -O optimization :(.

At this stage I wonder if we shouldn't extract this logics into separate dedicated scripts ? From what I see most projects are actually running python without any optimization specified so only the first command is useful to them.
The second command would only be useful to code being run with -OO switch.

I see the following leads:

  • Remove the second run.
    • according to the documentation, only the initial loading time of projects using that switch would be impacted
  • Remove the second run and add a script which could be run to force the compileall to be run with optimization parameters being provided as arguments
    • Would need being run at install time by projects requiring it (but then it would need to cache the various optimization requirements asked to rerun those upon upgrade)
    • Would need being run at startup time by projects requiring it

WDYT ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Now I understand. We at SABnzbd do run with -OO so it is usefull. I didn't know it created seperate files on each run.
Thanks for letting me know :)

Copy link

Choose a reason for hiding this comment

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

@Safihre Thanks, I was close to ask for known usages of -OO runtimes in my last reply. But since this is a generic python package, we would never know ;-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, since 3.8 they don't "overlap" each other. Hence the fact that in prior versions only two runs were "necessary". (see https://www.python.org/dev/peps/pep-0488/ for more details)

Copy link
Contributor

Choose a reason for hiding this comment

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

Indeed, the python38 package does not create *.pyo files. It creates *.pyc and *.opt-2.pyc files as by PEP 488.

As long as none of the packages runs python with -O we don't need to create respective bytecode at installation.

Copy link

Choose a reason for hiding this comment

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

Just to prevent future misunderstandings: pep-0488 has been implemented already in python 3.5, not 3.8 (https://bugs.python.org/issue23731). So this is not an issue regarding 3.7->3.8 upgrade.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status/published Published and activated (may take up to 48h until visible in DSM package manager)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Upgrade request for Python 3.7 to 3.8
6 participants