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

wheel.mk: Redesign proposal #4951

Merged
merged 82 commits into from
Nov 25, 2021
Merged

Conversation

th0ma7
Copy link
Contributor

@th0ma7 th0ma7 commented Nov 3, 2021

Motivation: Relates to the python wheel fix (e.g. 0.0.0 and invalid version found), after a few exchanges was suggested to redesign python wheel process.

Linked issues: #4921

Overall design

This proposal is a first attempt at this which does the following:

  1. Allows using any filename (thus backward compatible with existing requirements.txt)
  2. Allow using distinct files such that if the filename is requirements-pure.txt then its content is considered pure.
  3. Same behavior applies to requirements-cross.txt as cross (similar behavior as now)
  4. Allow using pure and/or cross prefix to python wheel definitions (with exception of dedicated files)
  5. Allow using WHEELS variable with direct wheel names, with or without prefix (no-prefix defaults to `pure)
  6. Allow re-defining WHEEL_DEFAULT_PREFIX variable for no-prefix (defaults set to cross unless ARCH is undefined wheras it gets defined to pure)

How it works

When gathering all wheels to be processed it generates:

  • wheelhouse/requirement-pure.txt
  • wheelhouse/requirement-cross.txt
  • wheelhouse/requirement-abi3.txt -> This allows generating cross-compiled wheels with limited python API/ABI

It then calls pip once per file to be processed (e.g. cross and abi3 using pip from crossenv vs pure using pip from native) followed by the creation of a unique requirements.txt file containing all wheels for packaging in staging/share/wheelhouse used at package installation time.

WHEEL_DEFAULT_PREFIX

At the lower-level, by default the WHEEL_DEFAULT_PREFIX is always set to cross. This means that no matter what the filename is to define requirements (except requirement-pure.txt) it will always cross-compile. Although when TC_ARCH is undefined, the value of WHEEL_DEFAULT_PREFIX gets flipped to pure no-arch type packages.

Finding details no. 1

@publicarray found out that call pip instead of $(PIP) solved a specific use case he had with sickchill (ref.: https://github.com/SynoCommunity/spksrc/pull/4920/files#r743494756).

I started investigating and ended-up noticing the pip difference between the calls where it would either point to system, crossenv or native but ALL where redirected to default pip which fallback to native (if native was indeed compiled).

After testing I now believe that most (but not all) failures where caused by this in the first place where using "native" pip would make "cross-compiled" wheels to fail as using a pip outside from the crossenv.

As such I've taken this a little further and was able to build 99% of sickchill wheels using only cross-compiling, while redirecting the 1% left over that still fails to using default native pip. This is a rather different approach than prior (e.g. dividing all "pure" vs "cross") where in this case everything is "cross" by default and only the ones which fails (a handful) is being redirected to use "native" and compiled as "pure-python" wheels.

Just to confirm my theory, I've played with python310, octoprint, beets and sickchill and remixed the wheels into a single requirements.txt where the handful "pure-python" uses the pure prefix.

Finding details no. 2

I made a new step into simplifying the wheel requirement management: adding poetry to the crossenv!
Immediately I was able to transform sickchill into a single & entirely cross-compile requirement file.
@hgy59 Yes there is still a need to manage pure vs cross wheels but the remaining "pure" exceptions is now really really thin.

TODO

  • Update the wiki
  • Allow passing pure and cross prefix to direct wheel calls into WHEELS variable (for DEBUG purpose)
  • avoid the prefixes always (unless used for debug purpose, see above)
  • only support requirements files and not packages for the WHEELS-variables.
  • Allow re-using existing requirements.txt
  • Allow using distinct files for pure vs cross (@publicarray)
  • allow building abi3 wheels (e.g @hgy59 fix the rename cross compiled wheels as *none-any.whl)
  • solve the workaround for the wrong arch name (@hgy59)

Optional - Migrate packages to python 3.10

Package testing

  • beets - kvmx64-7.0 - installation: NO ERRORS, basic testing: OK
  • borgbackup: - kvmx64-7.0 - installation: NO ERRORS, basic testing: OK
  • duplicity: - kvmx64-7.0 - installation: NO ERRORS, basic testing: OK
  • flexget - kvmx64-7.0 - installation: NO ERRORS, basic testing: need to understand configuration 1st
  • homeassistant - kvmx64-7.0 - installation: NO ERRORS, basic testing: OK
  • mercurial - kvmx64-7.0 - installation: NO ERRORS, basic testing: OK
  • python2 - kvmx64-7.0 - installation: NO ERRORS, basic testing: OK
  • python3 - kvmx64-7.0 - installation: NO ERRORS, basic testing: OK
  • python38 - kvmx64-7.0 - installation: NO ERRORS, basic testing: OK
  • python310 - kvmx64-7.0 - installation: NO ERRORS, basic testing: OK
  • octoprint - kvmx64-7.0 - installation: NO ERRORS, basic testing: web portal setup wizard startup OK
  • rdiff-backup - kvmx64-7.0 - installation: NO ERRORS, basic testing: OK
  • sabnzbd - kvmx64-7.0 - installation: NO ERRORS, basic testing: OK
  • sickchill - kvmx64-7.0 - installation: NO ERRORS, basic testing: OK
  • znc - kvmx64-7.0 - installation: NO ERRORS, basic testing: OK

Checklist

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

@th0ma7
Copy link
Contributor Author

th0ma7 commented Nov 3, 2021

@hgy59 to initiate discussions I've only focused on python310 as it isn't yet in usage by any package. If we come up with a design that looks fine then we could look into adapting all packages for using this.

Note that this proposal ended-up being much more straight-forward than expected to implement. I suggest to pay attention to:

  1. python310 Makefile WHEELS variable definition
  2. requirements.txt file where there is a mixture of pure, cross and no-prefix being used.

Please @Safihre , @publicarray, @ymartin59 and other fellow devs, feel free to join-in, inputs more than welcomed!

@publicarray
Copy link
Member

@Safihre
Copy link
Contributor

Safihre commented Nov 3, 2021

Still wonder if this is not just a bug in pip itself that it breaks for pure wheels if cross compiled. We are doing a lot of work here and it might be as simple as getting the base problem fixed at pip.. Isn't that worth checking?

@th0ma7
Copy link
Contributor Author

th0ma7 commented Nov 3, 2021

@th0ma7
Copy link
Contributor Author

th0ma7 commented Nov 3, 2021

Still wonder if this is not just a bug in pip itself that it breaks for pure wheels if cross compiled. We are doing a lot of work here and it might be as simple as getting the base problem fixed at pip.. Isn't that worth checking?

It certainly is worth checking. Still, in the meantime the issue as been undergoing since quite a while, well hidden and needs a workaround it.

@th0ma7 th0ma7 mentioned this pull request Nov 3, 2021
3 tasks
pure:html5lib==1.1
pure:idna==3.3
pure:importlib-metadata==4.8.1
pure:importlib-resources==5.3.0
Copy link
Member

@publicarray publicarray Nov 3, 2021

Choose a reason for hiding this comment

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

Since I'm a python noob I just want to comment on the style, I would prefer 2 separate files rather than prefixing. I know some text editors make is easier but still that's still a lot of text to change. It also departs what is otherwise a valid requirements.txt that developers are already familiar with. Alternatively maybe only use the cross prefix and default to pure? IDK just my 2c

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good points. This made me thinking, could we do both?

Commit below c9f1ef0 tries to alleviate this by:
a. allow using distinct files such that if the filename is requirements-pure.txt then consider it pure.
b. same behaviour applies to requirements-cross.txt as cross (similar to current)
c. for anything else, consider using cross|pure prefixes, if no prefix fallback to pure

This allows keeping already modified files to stay "as-is" because:

  1. requirements.txt was used for pure with no prefix with recent changes, so still good to go using catch-all c) and sort of as backward compatible as we can get
  2. requirements-cross.txt behavior stays as it is
  3. and it allows a mixture of possibilities to play with to get things to work as you'd like where you can:
  • Use the WHEELS variable with or direct package naming, with or without prefix
  • Use the filename you desire (using prefix for non-default)
  • Change default prefix
  • etc.

Copy link
Member

Choose a reason for hiding this comment

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

@th0ma7 🤩 Thanks 👍

@th0ma7
Copy link
Contributor Author

th0ma7 commented Nov 4, 2021

@hgy59 surprisingly it actually rebased like a charm! If you can comment on the proposal once you have a few spare cycles would be much appreciated, cheers!

@hgy59
Copy link
Contributor

hgy59 commented Nov 5, 2021

some of my suggestions

  • avoid the prefixes and use different requirements files for pure and cross (as @publicarray requested)
  • we should only support requirements files and not packages for the WHEELS-variables. Some times ago I moved all package==version definitions into requiements files (but we never forced an error for such definitions)
  • it might be a problem that we rename cross compiled wheels as *none-any.whl. This is an issue with pycryptodome and pycryptodomex (see homeassistant service-setup.sh) as those must have the "abi3" in the wheels name. But on the other side I left the none-any name in homeassistant and rename the cross compiled wheels at installation time before installing the wheels from whl files. This is also a workaround for the wrong arch name in the cross compiled wheels for armv7 (the wheels have arm instead of armv7l in the wheels name before renaming in spksrc.wheels.mk.
  • in homeassistant only cross compiled wheels are in the package, pure wheels are downloaded at installation time
  • if we cannot fix the cross wheels name and must rename at installation time, it might be better to have different wheels folders in the packages for cross and pure wheels, otherwise it is not possible to distingish those.
  • fixing wheel names at installation is easy as the arch must match uname -m on the target platform.

I give this collection of information here, as I will be offline for at least next week...

@th0ma7 th0ma7 mentioned this pull request Nov 6, 2021
3 tasks
@miigotu
Copy link
Contributor

miigotu commented Nov 6, 2021

I did not read the whole thread, but is it possible to use pyproject.toml instead of requirements.txt entirely (PEP518)?

Why can't common wheels be built and stored in a repository, and pulled into the build for each project?
See:
https://github.com/alpine-wheels/index
https://wheel-index.linuxserver.io/ubuntu/
https://wheel-index.linuxserver.io/alpine/
https://www.piwheels.org/simple (https://github.com/piwheels/piwheels)

I would much like to see the build system not rely solely on setuptools. We use poetry entirely for the whole update, build, and release process and having that as an option would be great. You could poetry build our wheel, then poetry export the requirements if necessary, then pip download the requirements from your index server (or git repository like the linuxserver one does) and zip up the spk and control files. Whole process would take a few seconds as long as there isn't a new version of a c-ext to be built.

@th0ma7
Copy link
Contributor Author

th0ma7 commented Nov 9, 2021

I did not read the whole thread, but is it possible to use pyproject.toml instead of requirements.txt entirely (PEP518)?

Not sure if it is fully ready, reading on it mentions it can't install dependencies yet with it (but really I'm no close to be a pro in this).

Why can't common wheels be built and stored in a repository, and pulled into the build for each project? See: https://github.com/alpine-wheels/index https://wheel-index.linuxserver.io/ubuntu/ https://wheel-index.linuxserver.io/alpine/ https://www.piwheels.org/simple (https://github.com/piwheels/piwheels)

@miigotu that is a really interesting option indeed... Really not sure how to do that but that could be promising.

@miigotu
Copy link
Contributor

miigotu commented Nov 9, 2021

I did not read the whole thread, but is it possible to use pyproject.toml instead of requirements.txt entirely (PEP518)?

Not sure if it is fully ready, reading on it mentions it can't install dependencies yet with it (but really I'm no close to be a pro in this).

Why can't common wheels be built and stored in a repository, and pulled into the build for each project? See: https://github.com/alpine-wheels/index https://wheel-index.linuxserver.io/ubuntu/ https://wheel-index.linuxserver.io/alpine/ https://www.piwheels.org/simple (https://github.com/piwheels/piwheels)

@miigotu that is a really interesting option indeed... Really not sure how to do that but that could be promising.

What can't install dependencies with pyproject? poetry sure can, currently spksrc cannot (but should). Setuptools is going to be replaced or fully pyproject.toml supported sooner than later imo.

If you download Sickchill from got, you poetry install and you have the package built and all dependencies installed along with the package, in a fresh virtualenv. poetry upgrade upgrades all the dependencies in that venv.

@publicarray
Copy link
Member

Setuptools is going to be replaced or fully pyproject.toml supported sooner than later imo.

After reading https://venthur.de/2021-06-26-python-packaging.html, I think there are going to be a few separate package managers for the foreseeable feature. I would love everything to be poetry but as long as the community hasn't chosen it as the main one... the python dependency saga continues.

@th0ma7
Copy link
Contributor Author

th0ma7 commented Nov 10, 2021

I consider the PR ready for a first review. Many additional issues or caveats that where hit due to:

  1. the wrong native pip called instead of cross
  2. as well as fixing many wheels by including by default in python3* default crossenv the infamous poetry.

I'm pretty sure additional hickup might emerge from the github-action but I think this is getting closer.

@@ -157,7 +157,7 @@ download_target: $(PRE_DOWNLOAD_TARGET)
else \
$(MSG) " wget $${url}" ; \
rm -f $${localFile}.part ; \
wget --secure-protocol=TLSv1_2 -nv -O $${localFile}.part -nc $${url} ; \
wget --secure-protocol=TLSv1_2 --no-check-certificate -nv -O $${localFile}.part -nc $${url} ; \
Copy link
Member

@publicarray publicarray Nov 10, 2021

Choose a reason for hiding this comment

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

Please put the genie back in the bottle. ⛔

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed!!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good call, and no worry, this is temporary as there was a download failure due to a invalid certificate for native/yasm thus blocking the build.

Copy link
Member

Choose a reason for hiding this comment

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

Ok thanks for the additional info, I mean you got to do what you got to do (for testing). But I really don't want it anywhere close to the main branch.

Copy link
Member

@publicarray publicarray Nov 10, 2021

Choose a reason for hiding this comment

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

Is it https://www.tortall.net/ with the expired cert?

Copy link
Member

Choose a reason for hiding this comment

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

It's probably better to switch to a GitHub release then: https://github.com/yasm/yasm/releases

Copy link
Member

Choose a reason for hiding this comment

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

I opened an issue just in case: yasm/yasm#190

Copy link
Contributor

Choose a reason for hiding this comment

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

This seems to be a recurring problem for them, so they suggest to use the GitHub release: yasm/yasm#182 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can someone tackle this (will probably offline for a few days)? I'll rebase this PR afterwards.

@th0ma7
Copy link
Contributor Author

th0ma7 commented Nov 10, 2021

It built successfully! woot! now time to start testing the packages...

What's next (from @hgy59's list):

  • avoid the prefixes
  • use different requirements files for pure and cross (as @publicarray requested)
  • only support requirements files and not packages for the WHEELS-variables.
  • fix the rename cross compiled wheels as *none-any.whl (ref pycryptodome and pycryptodomex in homeassistant service-setup.sh as those must have the "abi3" in the wheels name)
  • solve the workaround for the wrong arch name in the cross compiled wheels for armv7 (the wheels have arm instead of armv7l in the wheels name before renaming in spksrc.wheels.mk.
  • use homeassistant related cross compiled wheel's naming fixing at installation time must match uname -m on the target platform.
  1. As for the prefix specifically I'm ok for not using it anywhere. Although I would suggest leaving the functionality around as this saved me a lot of efforts while debugging packages.
  2. item 2 (individual files): TODO, very straight forward
  3. item 3 (WHEELS): TODO
  4. item 4-5-6 (wheel naming): requires removing change that kicked ffmpeg to build as there isn't enough cycles to go over the building process for all arches. Proposals:
  • create a spksrc.wheels-env.mk where definitions gets moved there along with arch package prefix handling for renaming as necessary (or something similar)
  • for the abi3 packages, could we create a abi3 dedicated package file to identify them to rename as needed? or use a very specific prefix for them for the later renaming? (sharing ideas)

@th0ma7
Copy link
Contributor Author

th0ma7 commented Nov 22, 2021

@hgy59 I managed to get homeassistant to work well with Python 3.10. Tested on my DS918 (x64-apollolake).
I intentionally downgraded pillow to 8.2 as it was blocking homeassistant to update it locally afterwards.
And beets would benefit from having access to pillow but it is a no-arch package. So having it within python 3.10 solves that.

@th0ma7
Copy link
Contributor Author

th0ma7 commented Nov 22, 2021

@Safihre heads-up, I was able to build and test successfully sabnzbd against python 3.10. Can you try it out to confirm? Thnx in advance for your help :)

@th0ma7
Copy link
Contributor Author

th0ma7 commented Nov 22, 2021

@BKSteve heads-up, I was able to build and test successfully sickchill with this PR. Can you give it a shot to confirm? thnx in advance!

@th0ma7
Copy link
Contributor Author

th0ma7 commented Nov 23, 2021

@ymartin59 with limited changes I was able to get beets to run with Python 3.10.

 /usr/local/bin/beet --version
beets version 1.5.0
Python version 3.10.0
no plugins loaded

@th0ma7
Copy link
Contributor Author

th0ma7 commented Nov 23, 2021

Hi all @SynoCommunity/developers , I now consider this PR ready for merge.

I propose merging this as-is and publish only python310 package for a start. I'd leave you to review/update packages you maintain as you see fit as you might want to take the opportunity to update or make additional changes.

Due to the changes to cross/pure/abi3 wheels there will probably some side-effects to packages not in scope of this PR build that I could not test. Remaining packages that I intend to convert in a subsequent PR #4965 that I've started working on :

  • plexpy
  • fishnet
  • rutorrent
  • salt-minion
  • tvheadend
  • bazarr

Please let me know if you see an issue with this proposal and have an alternate approach we can use.

EDIT: Wiki now fully updated to reflect upcoming changes with this PR.

@hgy59 hgy59 merged commit ce2283f into SynoCommunity:master Nov 25, 2021
@th0ma7 th0ma7 deleted the python-wheels-redesign branch December 17, 2021 10:35
publicarray pushed a commit to publicarray/spksrc that referenced this pull request Dec 19, 2021
* wheel.mk: Redesign proposal based on @hgy59 feedbacks
* native/python310: Downgrade crossenv 1.0 to fix missing pip
* wheel.mk: Allow passing prefix to wheel=x.y in WHEELS variable
* python310: Add wheel=x.y to WHEELS variable to showcase
* wheel.mk: Adjust identation
* native/python310: Revert back to crossenv==1.1.4
* cross/wheels*: Update pip and setuptools to latest versions
* native/python310: Revert back to crossenv==1.1.4
* wheel.mk: Allow using requirements-pure|cross.txt files
* wheel.mk: Enfore pure|cross on dedicated files
* python310: Add poetry and remote pure: to use default behavior
* python310: Add regex and netifaces
* pillow: Downgrading to 8.2.0 per @hgy59 request
* beets: Update to python310
* tvheadend: Migrate to python310
* python310: Update requirements using cross as default
* python310: Remove temporary examples in WHEELS definition
* wheel.mk: Fix cross-compiling (based on @publicarray finding)
* python3(legacy): Align pip and setuptools with python310
* octoprint: Update to python310
* sickchill: Update to python310
* tvheadend: Remove python virtualenv as all wheels in python310
* octoprint: Migrate service-setup to use python310
* sickchill: Update service-setup to use python310
* beets: Update service-setup to use python310
* flexget: Migrate to python310
* python3: Adjust requirements.txt
* python38: Adjust requirements
* sabnzbd: Update to python 3.10
* download.mk: Temporary add --no-certificate-check to wget for yasm
* beets: Update native pip path for python310
* duplicity: Migrate to python 3.10
* borgbackup: Migrate to python 3.10
* deluge: Adjust requirements to latest changes
* python310: Add poetry to crossenv - e.g. ONE FIX TO RULE THEM ALL
* python310: Rework requirements with poetry now in crossenv
* sickchill: All wheels are now cross thnx to poetry in crossenv
* python310: Move basic wheels from WHEELS to requirements.txt
* octoprint: Migrate pure to cross wheels using poetry
* sabnzbd: Migrate additional pure to cross wheels due to poetry
* flexget: Migrate pure to cross requirements due to poetry
* rdiff-backup: Migrate to python310
* python3: Align with findings from python310
* python38: Align with findings from python310
* mercurial: Migrate to python310
* ffsync: Merge back into a single requirement.txt file
* beets: Fix build issue with python-mdp2
* tvheadend: Revert changes to stop building ffmpeg (ref: SynoCommunity#4965)
* python310: Allow building wheels with limited API (abi3 prefix)
* python310: pycrypto is no longer maintained
* wheel.mk: Manage arch specific file prefixes
* pillow: Re-updating to version 8.4.0 per @hgy59 request
* download.mk: Remove temporary no-certificate-check
* wheel.mk: Fix naming for ARMv5 88f6281 as armv5tel
* wheel.mk: Only support requirement files in WHEELS variable
* borgbackup: Use cross & pure requirement files
* duplicity: Use cross & pure requirement files
* flexget: Use cross & pure requirement files
* octoprint: requirement.txt entirely managed through cross-compiled
* python38: Use cross & pure requirement files
* python3: Use cross & pure requirement files
* sabnzbd: Use cross & pure requirement files
* python310: Remove forgotten pure wheels from cross requirement
* sickchill: Bump package version for python 3.10 migration
* python310: Remove cryptography from abi3 (and keep in cross)
* homeassistant: Rough 1st pass to migrate to python310
* Update cython & numpy to latest version to fix x86_64 builds
* python310: Add pycares
* homeassistant: Misc cleanups
* homeassistant: Now fully functional with Python 3.10
* sickchill: Fix typo
* sabnzbd: pytz wheel already provided by python310
* sickchill: BUILD_DEPENDS to python310
* octoprint: Small adjustments to get clean install using python310
* flexget: Disable python310 default wheels
* beets: Now compatible with python310 with update to 1.5.0
* borgbackup: Misc fixes for python310 compatibility
* duplicity: Misc fixes for python310 compabitility
* duplicity: Disable future wheel as already in python310
* znc: Migrate to Python 3.10
* python-wheel.mk: Rename using PYTHON_ARCH instead of none-any.whl
* Rename requirements-cross.txt to requirements-crossenv.txt
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants