tomerfiliba-org / reedsolomon

⏳🛡 Pythonic universal errors-and-erasures Reed-Solomon codec to protect your data from errors and bitrot. Includes a future-proof zero-dependencies pure-python implementation 🔮 and an optional speed-optimized Cython/C extension 🚀
http://pypi.python.org/pypi/reedsolo
Other
351 stars 86 forks source link

1.7.0: pep517 is not building DSO `creedsolo` module #57

Closed kloczek closed 1 year ago

kloczek commented 1 year ago

Looks like something is wrog when pep517 is used and DSO modules is not build

```console + /usr/bin/python3 -sBm build -w --no-isolation * Getting build dependencies for wheel... running egg_info creating reedsolo.egg-info writing reedsolo.egg-info/PKG-INFO writing dependency_links to reedsolo.egg-info/dependency_links.txt writing top-level names to reedsolo.egg-info/top_level.txt writing manifest file 'reedsolo.egg-info/SOURCES.txt' reading manifest file 'reedsolo.egg-info/SOURCES.txt' reading manifest template 'MANIFEST.in' adding license file 'LICENSE' writing manifest file 'reedsolo.egg-info/SOURCES.txt' * Building wheel... running bdist_wheel running build running build_py creating build creating build/lib copying reedsolo.py -> build/lib installing to build/bdist.linux-x86_64/wheel running install running install_lib creating build/bdist.linux-x86_64 creating build/bdist.linux-x86_64/wheel copying build/lib/reedsolo.py -> build/bdist.linux-x86_64/wheel running install_egg_info running egg_info writing reedsolo.egg-info/PKG-INFO writing dependency_links to reedsolo.egg-info/dependency_links.txt writing top-level names to reedsolo.egg-info/top_level.txt reading manifest file 'reedsolo.egg-info/SOURCES.txt' reading manifest template 'MANIFEST.in' adding license file 'LICENSE' writing manifest file 'reedsolo.egg-info/SOURCES.txt' Copying reedsolo.egg-info to build/bdist.linux-x86_64/wheel/reedsolo-1.7.0-py3.8.egg-info running install_scripts creating build/bdist.linux-x86_64/wheel/reedsolo-1.7.0.dist-info/WHEEL creating '/home/tkloczko/rpmbuild/BUILD/reedsolomon-1.7.0/dist/.tmp-h1l51r67/reedsolo-1.7.0-py3-none-any.whl' and adding 'build/bdist.linux-x86_64/wheel' to it adding 'reedsolo.py' adding 'reedsolo-1.7.0.dist-info/LICENSE' adding 'reedsolo-1.7.0.dist-info/METADATA' adding 'reedsolo-1.7.0.dist-info/WHEEL' adding 'reedsolo-1.7.0.dist-info/top_level.txt' adding 'reedsolo-1.7.0.dist-info/RECORD' removing build/bdist.linux-x86_64/wheel Successfully built reedsolo-1.7.0-py3-none-any.whl ```
lrq3000 commented 1 year ago

Hello, thank you for your feedback. If by DSO you mean the creedsolo cythonized extension, then it's because now it's not installed by default, you have to supply a flag on install to enable it. I don't know how to do that with pep517, but with the usual pip you can do:

pip install --upgrade reedsolo --install-option="--cythonize"

5 mars 2023 11:18:38 Tomasz Kłoczko @.***>:

Looks like something is wrog when pep517 is used and DSO modules is not build

  • /usr/bin/python3 -sBm build -w --no-isolation
  • Getting build dependencies for wheel... running egg_info creating reedsolo.egg-info writing reedsolo.egg-info/PKG-INFO writing dependency_links to reedsolo.egg-info/dependency_links.txt writing top-level names to reedsolo.egg-info/top_level.txt writing manifest file 'reedsolo.egg-info/SOURCES.txt' reading manifest file 'reedsolo.egg-info/SOURCES.txt' reading manifest template 'MANIFEST.in' adding license file 'LICENSE' writing manifest file 'reedsolo.egg-info/SOURCES.txt'
  • Building wheel... running bdist_wheel running build running build_py creating build creating build/lib copying reedsolo.py -> build/lib installing to build/bdist.linux-x86_64/wheel running install running install_lib creating build/bdist.linux-x86_64 creating build/bdist.linux-x86_64/wheel copying build/lib/reedsolo.py -> build/bdist.linux-x86_64/wheel running install_egg_info running egg_info writing reedsolo.egg-info/PKG-INFO writing dependency_links to reedsolo.egg-info/dependency_links.txt writing top-level names to reedsolo.egg-info/top_level.txt reading manifest file 'reedsolo.egg-info/SOURCES.txt' reading manifest template 'MANIFEST.in' adding license file 'LICENSE' writing manifest file 'reedsolo.egg-info/SOURCES.txt' Copying reedsolo.egg-info to build/bdist.linux-x86_64/wheel/reedsolo-1.7.0-py3.8.egg-info running install_scripts creating build/bdist.linux-x86_64/wheel/reedsolo-1.7.0.dist-info/WHEEL creating '/home/tkloczko/rpmbuild/BUILD/reedsolomon-1.7.0/dist/.tmp-h1l51r67/reedsolo-1.7.0-py3-none-any.whl' and adding 'build/bdist.linux-x86_64/wheel' to it adding 'reedsolo.py' adding 'reedsolo-1.7.0.dist-info/LICENSE' adding 'reedsolo-1.7.0.dist-info/METADATA' adding 'reedsolo-1.7.0.dist-info/WHEEL' adding 'reedsolo-1.7.0.dist-info/top_level.txt' adding 'reedsolo-1.7.0.dist-info/RECORD' removing build/bdist.linux-x86_64/wheel Successfully built reedsolo-1.7.0-py3-none-any.whl

— Reply to this email directly, view it on GitHub[https://github.com/tomerfiliba/reedsolomon/issues/57], or unsubscribe[https://github.com/notifications/unsubscribe-auth/AAIRFXTG5VL2S22S4RCTOY3W2RR73ANCNFSM6AAAAAAVQCRPGE]. You are receiving this because you are subscribed to this thread.[Image de pistage][https://github.com/notifications/beacon/AAIRFXVLXVQS6SL3XEQ6XUDW2RR73A5CNFSM6AAAAAAVQCRPGGWGG33NNVSW45C7OR4XAZNFJFZXG5LFVJRW63LNMVXHIX3JMTHF76F4TY.gif]

kloczek commented 1 year ago

Hello, thank you for your feedback. If by DSO you mean the creedsolo cythonized extension,

Yes. DSO stands for Dynamic Shared Object.

then it's because now it's not installed by default, you have to supply a flag on install to enable it. I don't know how to do that with pep517, but with the usual pip you can do: pip install --upgrade reedsolo --install-option="--cythonize"

This issuw is not about installation of the reedsolo out of .whl archive downloaded from pypi repo. It is about building reedsolo out of source tree.

lrq3000 commented 1 year ago

Yes, the flag enables the compilation of the creedsolo extension, as it is not included in the wheel anymore (the sourcecode of creedsolo is however).

I have no experience with pep517 whatsoever, it's the first time I hear about it. Can you please quickly run me down on how to reproduce your issue? Do I need to install a module for pep517?

5 mars 2023 14:34:30 Tomasz Kłoczko @.***>:

Hello, thank you for your feedback. If by DSO you mean the creedsolo cythonized extension,

Yes. DSO stands for Dynamic Shared Object.

then it's because now it's not installed by default, you have to supply a flag on install to enable it. I don't know how to do that with pep517, but with the usual pip you can do: pip install --upgrade reedsolo --install-option="--cythonize"

This issuw is not about installation of the reedsolo out of .whl archive downloaded from pypi repo. It is about building reedsolo out of source tree.

— Reply to this email directly, view it on GitHub[https://github.com/tomerfiliba/reedsolomon/issues/57#issuecomment-1455093732], or unsubscribe[https://github.com/notifications/unsubscribe-auth/AAIRFXS63VCLFEV2LF76BHTW2SI6LANCNFSM6AAAAAAVQCRPGE]. You are receiving this because you commented.[Image de pistage][https://github.com/notifications/beacon/AAIRFXXNQT6ELWZGZBZSGPDW2SI6LA5CNFSM6AAAAAAVQCRPGGWGG33NNVSW45C7OR4XAZNMJFZXG5LFINXW23LFNZ2KUY3PNVWWK3TUL5UWJTSWXL36I.gif]

kloczek commented 1 year ago

OK looks like I was able to build with DSO module however build shows deprecation warnings.

+ /usr/bin/python3 -sBm build -w --no-isolation -C=--global-option=--cythonize
* Getting build dependencies for wheel...
running egg_info
creating reedsolo.egg-info
writing reedsolo.egg-info/PKG-INFO
writing dependency_links to reedsolo.egg-info/dependency_links.txt
writing top-level names to reedsolo.egg-info/top_level.txt
writing manifest file 'reedsolo.egg-info/SOURCES.txt'
reading manifest file 'reedsolo.egg-info/SOURCES.txt'
reading manifest template 'MANIFEST.in'
adding license file 'LICENSE'
writing manifest file 'reedsolo.egg-info/SOURCES.txt'
* Building wheel...
Cython is installed, building creedsolo module
[1/1] Cythonizing creedsolo.pyx
running bdist_wheel
running build
running build_py
creating build
creating build/lib.linux-x86_64-cpython-38
copying reedsolo.py -> build/lib.linux-x86_64-cpython-38
running build_ext
building 'creedsolo' extension
creating build/temp.linux-x86_64-cpython-38
/usr/bin/gcc -DDYNAMIC_ANNOTATIONS_ENABLED=1 -DNDEBUG -O2 -g -grecord-gcc-switches -pipe -Wall -Werror=format-security -Wp,-U_FORTIFY_SOURCE,-D_FORTIFY_SOURCE=3 -Wp,-D_GLIBCXX_ASSERTIONS -fstack-protector-strong -fno-omit-frame-pointer -mno-omit-leaf-frame-pointer -m64 -mtune=generic -fasynchronous-unwind-tables -fstack-clash-protection -fcf-protection -fdata-sections -ffunction-sections -flto=auto -flto-partition=none -D_GNU_SOURCE -fPIC -fwrapv -ffat-lto-objects -D_GNU_SOURCE -fPIC -fwrapv -O2 -g -grecord-gcc-switches -pipe -Wall -Werror=format-security -Wp,-U_FORTIFY_SOURCE,-D_FORTIFY_SOURCE=3 -Wp,-D_GLIBCXX_ASSERTIONS -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -fstack-protector-strong -specs=/usr/lib/rpm/redhat/redhat-annobin-cc1 -fno-omit-frame-pointer -mno-omit-leaf-frame-pointer -m64 -mtune=generic -fasynchronous-unwind-tables -fstack-clash-protection -fcf-protection -fdata-sections -ffunction-sections -flto=auto -flto-partition=none -D_GNU_SOURCE -fPIC -fwrapv -ffat-lto-objects -O2 -g -grecord-gcc-switches -pipe -Wall -Werror=format-security -Wp,-U_FORTIFY_SOURCE,-D_FORTIFY_SOURCE=3 -Wp,-D_GLIBCXX_ASSERTIONS -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -fstack-protector-strong -specs=/usr/lib/rpm/redhat/redhat-annobin-cc1 -fno-omit-frame-pointer -mno-omit-leaf-frame-pointer -m64 -mtune=generic -fasynchronous-unwind-tables -fstack-clash-protection -fcf-protection -fdata-sections -ffunction-sections -flto=auto -flto-partition=none -D_GNU_SOURCE -fPIC -fwrapv -ffat-lto-objects -O2 -g -grecord-gcc-switches -pipe -Wall -Werror=format-security -Wp,-U_FORTIFY_SOURCE,-D_FORTIFY_SOURCE=3 -Wp,-D_GLIBCXX_ASSERTIONS -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -fstack-protector-strong -specs=/usr/lib/rpm/redhat/redhat-annobin-cc1 -fno-omit-frame-pointer -mno-omit-leaf-frame-pointer -m64 -mtune=generic -fasynchronous-unwind-tables -fstack-clash-protection -fcf-protection -fdata-sections -ffunction-sections -flto=auto -flto-partition=none -fPIC -I/usr/include/python3.8 -c creedsolo.c -o build/temp.linux-x86_64-cpython-38/creedsolo.o
/usr/bin/gcc -shared -Wl,--as-needed -Wl,--gc-sections -Wl,-z,now -specs=/usr/lib/rpm/redhat/redhat-hardened-ld -flto=auto -flto-partition=none -fuse-linker-plugin -Wl,--build-id=sha1 -Wl,--as-needed -Wl,--gc-sections -Wl,-z,now -specs=/usr/lib/rpm/redhat/redhat-hardened-ld -flto=auto -flto-partition=none -fuse-linker-plugin -Wl,--build-id=sha1 -Wl,--as-needed -Wl,--gc-sections -Wl,-z,now -specs=/usr/lib/rpm/redhat/redhat-hardened-ld -flto=auto -flto-partition=none -fuse-linker-plugin -Wl,--build-id=sha1 -O2 -g -grecord-gcc-switches -pipe -Wall -Werror=format-security -Wp,-U_FORTIFY_SOURCE,-D_FORTIFY_SOURCE=3 -Wp,-D_GLIBCXX_ASSERTIONS -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -fstack-protector-strong -specs=/usr/lib/rpm/redhat/redhat-annobin-cc1 -fno-omit-frame-pointer -mno-omit-leaf-frame-pointer -m64 -mtune=generic -fasynchronous-unwind-tables -fstack-clash-protection -fcf-protection -fdata-sections -ffunction-sections -flto=auto -flto-partition=none build/temp.linux-x86_64-cpython-38/creedsolo.o -L/usr/lib64 -o build/lib.linux-x86_64-cpython-38/creedsolo.cpython-38-x86_64-linux-gnu.so
installing to build/bdist.linux-x86_64/wheel
running install
running install_lib
creating build/bdist.linux-x86_64
creating build/bdist.linux-x86_64/wheel
copying build/lib.linux-x86_64-cpython-38/reedsolo.py -> build/bdist.linux-x86_64/wheel
copying build/lib.linux-x86_64-cpython-38/creedsolo.cpython-38-x86_64-linux-gnu.so -> build/bdist.linux-x86_64/wheel
running install_egg_info
running egg_info
writing reedsolo.egg-info/PKG-INFO
writing dependency_links to reedsolo.egg-info/dependency_links.txt
writing top-level names to reedsolo.egg-info/top_level.txt
reading manifest file 'reedsolo.egg-info/SOURCES.txt'
reading manifest template 'MANIFEST.in'
adding license file 'LICENSE'
writing manifest file 'reedsolo.egg-info/SOURCES.txt'
Copying reedsolo.egg-info to build/bdist.linux-x86_64/wheel/reedsolo-1.7.0-py3.8.egg-info
running install_scripts
creating build/bdist.linux-x86_64/wheel/reedsolo-1.7.0.dist-info/WHEEL
creating '/home/tkloczko/rpmbuild/BUILD/reedsolomon-1.7.0/dist/.tmp-xsoyhpz6/reedsolo-1.7.0-cp38-cp38-linux_x86_64.whl' and adding 'build/bdist.linux-x86_64/wheel' to it
adding 'creedsolo.cpython-38-x86_64-linux-gnu.so'
adding 'reedsolo.py'
adding 'reedsolo-1.7.0.dist-info/LICENSE'
adding 'reedsolo-1.7.0.dist-info/METADATA'
adding 'reedsolo-1.7.0.dist-info/WHEEL'
adding 'reedsolo-1.7.0.dist-info/top_level.txt'
adding 'reedsolo-1.7.0.dist-info/RECORD'
removing build/bdist.linux-x86_64/wheel
/usr/lib/python3.8/site-packages/setuptools/build_meta.py:307: SetuptoolsDeprecationWarning:
            The arguments ['--cythonize'] were given via `--global-option`.
            Please use `--build-option` instead,
            `--global-option` is reserved to flags like `--verbose` or `--quiet`.

  warnings.warn(msg, SetuptoolsDeprecationWarning)
warning: creedsolo.pyx:202:15: Index should be typed for more efficient access
Successfully built reedsolo-1.7.0-cp38-cp38-linux_x86_64.whl
lrq3000 commented 1 year ago

Thank you for your feedback, happy to know it works!

About the missing typing especially of indices, it's true not everything was typed, I will push an update soon on git (but not as a release) with a few more typed definitions. I may do more cythonization in the future if I ever get some spare time for that.

About the --global-option warning, I have no idea where this comes from, as we use --install-option, and replacing it with --build-option does not work, so I guess it's an internal shenanigan of Setuptools that will be fixed in future updates.

kloczek commented 1 year ago

Neverteless pep517 build procedure is not obviouse. IMO it would be good to put one line about that in README.rst. Thx 👍

lrq3000 commented 1 year ago

Ah ok sorry I did not understand you had to type a specific command, I now noticed it.

I however cannot reproduce your environment, could you please tell me how I can use pep517? I tried to pip install pep517 but it doesn't have the -w and --no-isolation arguments. What implementation did you use to get pep517 working? I am using Python 3.10.8 currently via Miniconda3 (the latest Python release I could get my hands on in the conda packages manager).

kloczek commented 1 year ago

This is not abput pep517 module. build is crucial.

Here is list of installed modules in build env

```console Package Version --------------- -------------- attrs 22.2.0 build 0.10.0 Cython 0.29.33 distro 1.8.0 exceptiongroup 1.0.0 gpg 1.18.0-unknown iniconfig 2.0.0 libcomps 0.1.19 packaging 23.0 pip 23.0.1 pluggy 1.0.0 pyproject_hooks 1.0.0 pytest 7.2.2 python-dateutil 2.8.2 rpm 4.17.0 setuptools 65.6.3 six 1.16.0 tomli 2.0.1 wheel 0.38.4 ```
kloczek commented 1 year ago

You can skip rpm and gpg moduels from that list because they have been dragged by dnf used to form build env using rpm packages.

lrq3000 commented 1 year ago

Awesome, thank you so much @kloczek ! Doing a pip install build allowed me to use the commands you provided! I just changed the --global-option to --build-option to avoid the warning, and everything is fine now!

Also stay tuned in the future for updates on the cythonized extension, I want to eventually revisit it to complete the cythonization and get additional speed boosts, as it is clear we can squeeze a lot more than is currently obtained with some more optimizations!