gijzelaerr / python-snap7

A Python wrapper for the snap7 PLC communication library
http://python-snap7.readthedocs.org/
MIT License
643 stars 246 forks source link

imporove source-only install #406

Closed Cube707 closed 1 year ago

Cube707 commented 1 year ago

currently it is not posible to install this libary from source, only from wheel as they are packaged with the needed dll. However this is not obvious, as the install will complete and the libary "just not work"

I suggest imporoving setup.py:

from my point of vie, it would also be benifical to check the avalable pre-compiled libayes into the repo or host them on the docs-page. This would make the install/download much easier. Currently, the version is hard-coded anyway, so its not much of a difference.

Please tell me if you are interested in this.

gijzelaerr commented 1 year ago

hi @Cube707, thank you for your proposal.

What currently doesn't work with the client install? If the DLL is somewhere on your path during runtime the library should work. Are you trying to run python-snap7 on a platform that doesn't have a binary wheel? we can also try to add your build target.

I don't think setup.py should download anything, this is quite uncommon and unexpected behaviour for a setup script. Also, we might move to pyproject.tomlstructure completely at some point.

Compiling snap7 sounds like a nightmare and opening a can of worms. Pre-compiled binaries are supplied for most if not all platforms. Someone who wants to install python-snap7 from source probably also has the knowledge on how to obtain a snap7 DLL for his or her platform and make sure it can be found on the path.

Cube707 commented 1 year ago

Putting the dll on the path indeed is enough to use this libary and when installing the wheels it works all fine.

However, I am working towards implementing a custom s7 client to automatically test some s7-protocolls and need a custom , self compiled version of snap7 and also modifayed the sourcecod3e of this project.

It just strikes me a odd that a install via pip install . will succseed without any form of warning even if I don't have the dll and the project will clearly not work.

Also on systems where there is no wheel avalable on pypi.org pip will use the soruce-distro and run the setup.py, which will again succeed without any warning and than simply not work.

While this of course can be avoided by reading the docs and "knowing what your doing", these problems are easielie detectable during isntall and are therefore avoidable.

I suggest: defnetly raising a warning from setup.py if the install finishes and snap7.dll can not be found on PATH


You are right that downloading assets on install-time in typically not done. Instead all required assets are packaged with the release. This libary however currently does this in a verry hidden way, only on gihub-pipeline run and only for the wheel distros.

Making this more obvious could help, for example by providing the setup as a script that is located in a obvious position (python, Makefile, whatever)

gijzelaerr commented 1 year ago

unfortunatly the pypi stats don't include the wheel to tarball ratio:

https://pepy.tech/project/python-snap7

but my estimate is that about 99% of the downloads is wheels and just a fraction of the manual builds. I can repeat all my arguments above again, but that is not productive. I don't think the "power user" that wants a source install doesn't know how a DLL should be loaded, we also documented that.

But, if makes your life really significantly better, you can print a warning in a PEP 621 compatible way (we will get rid of the setup.py file at some point) then feel free to issue a PR.

Cube707 commented 1 year ago

I don't think there is a PEP621 compliant way to do install-time checks...

closing this