Allow for platform dependent builds

[SaWey]

Home Assistant has been having issues using this module in the last year(s).
The keep their own wheels repo and the automated system did not build platform dependent versions.
Since this module is building the adslib dependency in a special way, it goes unnoticed by setuptools.

The extra line added in this pull request mitigates the problem and creates a platform dependent filename for the wheel.

https://stackoverflow.com/a/64921892 (by CiaranWelsh):
Adding below line to setup arguments in setup.py

has_ext_modules=lambda: True

This let me build pyads-3.3.9-cp310-cp310-linux_x86_64.whl instead of pyads-3.3.9-py3-none-any.whl

[stlehmann]

@SaWey Sounds like a reasonable idea. However I couldn't find a documentation to has_ext_modules. Could you provide a link to it?

[SaWey]

Hi @stlehmann

It doesn't have documentation as it is probably an internal function not really meant to be used like this.
If you look at setuptools/_distutils/dist.py

    def has_ext_modules(self):
        return self.ext_modules and len(self.ext_modules) > 0

The function looks to see if there are any external modules, as documented here: https://setuptools.pypa.io/en/latest/userguide/ext_modules.html

To instruct setuptools to compile the foo.c file into the extension module
mylib.foo, we need to add a setup.py file similar to the following:

   from setuptools import Extension, setup

   setup(
       ext_modules=[
           Extension(
               name="mylib.foo",  # as it would be imported
                                  # may include packages/namespaces separated by `.`

               sources=["foo.c"], # all sources are compiled into a single binary file
           ),
       ]
   )

Since your library uses custom functions to build the ADS binary, setuptools fails to notice it is platform independent.
By overriding the has_ext_modules function, we have an easy workaround.

[BartDurnez]

Hi guys,

@stlehmann can you work on a solution with the info @SaWey provided ?

Another question, does the change in this pyads project has influence on the HA core builds?

[SaWey]

@BartDurnez once this change is published, HA needs to update its dependency to the latest version of this library before it will have an effect.

[martinius74]

Hi @stlehmann, @SaWey, @BartDurnez,
could someone make sure this change is approved and merged?
Can't wait to connect my HA to Beckhoff PLC :-)
Thanks,
Martin

[martinius74]

Hi @SaWey ..

This let me build pyads-3.3.9-cp310-cp310-linux_x86_64.whl instead of pyads-3.3.9-py3-none-any.whl

How can this custom wheel be installed be installed in my HA server?

[martinius74]

@stlehmann , thanks for merging this pull request!

[SaWey]

@stlehmann Thanks for the merge, any chance a new version can be released as well?

[drindal82]

It would be really great, if this would be released very soon.

[chrisbeardy]

since implementing this change, pytest appears to fial on GitHub CI:

@SaWey @drindal82 @martinius74 @BartDurnez you all seem quite motivated to have this released, any ideas how to fix this?

[SaWey]

since implementing this change, pytest appears to fial on GitHub CI:
...
@SaWey @drindal82 @martinius74 @BartDurnez you all seem quite motivated to have this released, any ideas how to fix this?

The error starts with installing the dependencies:
https://github.com/stlehmann/pyads/actions/runs/8169489197/job/22333671133#step:4:271

ERROR: pip's dependency resolver does not currently take into account all the packages that are installed. This behaviour is the source of the following dependency conflicts.
pytest 8.0.2 requires pluggy<2.0,>=1.3.0, but you have pluggy 1.0.0 which is incompatible.

When upgrading pluggy to version 1.3.0 my local test on python 3.10.12 is working:

====================================================================== test session starts ======================================================================
platform linux -- Python 3.10.12, pytest-8.0.2, pluggy-1.3.0
rootdir: /mnt/c/dev/pyads
configfile: tox.ini
testpaths: tests
plugins: cov-4.1.0
collected 108 items

tests/test_ads.py ........                                                                                                                                [  7%]
tests/test_connection_class.py ...............................................................                                                            [ 65%]
tests/test_filetimes.py ..                                                                                                                                [ 67%]
tests/test_plc_ams.py .                                                                                                                                   [ 68%]
tests/test_plc_route.py ..                                                                                                                                [ 70%]
tests/test_symbol.py ............................                                                                                                         [ 96%]
tests/test_testserver.py ..                                                                                                                               [ 98%]
tests/test_utils.py ..                                                                                                                                    [100%]

======================================================================= warnings summary ========================================================================
tests/test_plc_ams.py::PLCAMSTestCase::test_get_ams
  /mnt/c/dev/pyads/tests/test_plc_ams.py:41: DeprecationWarning: setDaemon() is deprecated, set the daemon attribute instead
    route_thread.setDaemon(True)

tests/test_plc_route.py::PLCRouteTestCase::test_correct_route
  /mnt/c/dev/pyads/tests/test_plc_route.py:158: DeprecationWarning: setDaemon() is deprecated, set the daemon attribute instead
    route_thread.setDaemon(True)

tests/test_plc_route.py::PLCRouteTestCase::test_incorrect_route
  /mnt/c/dev/pyads/tests/test_plc_route.py:183: DeprecationWarning: setDaemon() is deprecated, set the daemon attribute instead
    route_thread.setDaemon(True)

-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html
================================================================ 108 passed, 3 warnings in 8.35s ================================================================
s

[chrisbeardy]

ok, on it, will update...fool me for skipping pass the tick on the build report assuming it had installed dependencies OK

[drindal82]

any news on a release date?

[chrisbeardy]

I'm getting issues building across all python versions, it appears that to support pluggy version 1.3, python 3.7 is not supported. Python 3.7 is EOL so I'm happy to drop direct support for it (or at least testing on CI etc), however this needs further work to do in the repo to cater for this before it can be released, and I'm struggling to find the time right now.

[chrisbeardy]

I am however hoping things will free up in the next 4-6 weeks.

[drindal82]

ok thanks @chrisbeardy for the update. We are building our new house at the moment and I'm eager to start working with homeassistant and our twincat system. I will try to be more patient ;)

[chrisbeardy]

I would also get on to home assistant, as this package always used to work with it and as you can see we haven't made a release in a while so they must have changed something their end.

[SaWey]

It probably is the pytest lib causing the failed builds.
Pytest v8.0.0 was released 2024-01-27 (2 days after the last successful build), and it requires python >= v3.8 .

Would updating the github workflow this something like this work:

    - name: Install dependencies
      run: |
        python -m pip install --upgrade pip
        if [ ${{ matrix.python-version }} == '3.7' ]; then python -m pip install flake8 pytest coverage coveralls pytest-cov; fi
        if [ ${{ matrix.python-version }} != '3.7' ]; then python -m pip install flake8 pytest==7.4.4 coverage coveralls pytest-cov; fi
        if [ -f requirements.txt ]; then pip install -r requirements.txt; fi

[SaWey]

I would also get on to home assistant, as this package always used to work with it and as you can see we haven't made a release in a while so they must have changed something their end.

HA has changed the way they manage dependencies, which caused the problem on most systems.

[chrisbeardy]

Version 3.4.0 has just been released to pypi so hopefully this fixes the home assistant issue

[drindal82]

Any News? Does it work with the new pypi version?

[chrisbeardy]

Youll have to check in with home assistant for that to see when the new pyads pypi version will be integrated

[drindal82]

Ok thanks, but unfortunately I don't know how to do that. So I just have to wait.

[chrisbeardy]

Maybe open an issue in the home assistant repo?

[drindal82]

i'll try my best, thanks for the hint