ni / nisystemlink-clients-python

Python API for interacting with a SystemLink Server, created and supported by NI.
https://ni.com/systemlink
MIT License
10 stars 20 forks source link

Ensure nisystemlink-clients-python treats systemlink as an implicit namespace package #11

Closed alexweav closed 4 years ago

alexweav commented 4 years ago

What does this Pull Request accomplish?

systemlink is a PEP-420 style namespace package. This mechanism is used by Python-based SystemLink services, allowing the service code to live under the systemlink namespace, but exist elsewhere on the module search path, sandboxed outside of site-packages. After installing nisystemlink-clients-python, an __init__.py is dropped in the site-packages portion of the namespace, causing Python to ignore other packages within the namespace, elsewhere in the search path.

As documented:

It is extremely important that every distribution that uses the namespace package omits the init.py or uses a pkgutil-style init.py. If any distribution does not, it will cause the namespace logic to fail and the other sub-packages will not be importable.

Why should this Pull Request be merged?

This PR causes the __init__.py in the systemlink namespace to be omitted from the produced .whl, restoring the PEP-420 behavior.

There are numerous issues when using mypy on namespace packages, so the __init__.py in the systemlink directory is not actually removed as part of this PR. As far as mypy is concerned, this is a regular package. The setup.py is updated to treat systemlink as a namespace package, and that __init__.py is not included in the .whl.

What testing has been done?

Manually inspected the generated .whl to ensure that the file was not present. Installed the generated package and verified that other systemlink modules elsewhere on the search path were importable. Installed the generated package by itself on a fresh system and verified that it is importable.

alexweav commented 4 years ago

Interestingly, the change to tox.ini causes the py35 build to try to run ALL FOUR build environments. It then tries to run black on py35, which is unsupported, so that build fails. My change doesn't seem to be related to the build matrix functionality at all, confusingly - currently looking into a fix.

DavidCurtiss commented 4 years ago

.tox create: D:\a\nisystemlink-clients-python\nisystemlink-clients-python.tox.tox

It looks like adding a setup_requires causes it to run tox inside of tox. And the internal tox doesn't obey the gh_actions. I dunno why it decides to run tox inside of tox, though.

alexweav commented 4 years ago

It seems to be not the setup_requires, but rather the requires tag in the tox.ini. I've tried many combinations of setup_requires, install_requires, pyproject.toml, etc. Without the requires tag, the py35 build fails because setuptools is too old, and with it, we get the strange tox-within-tox behavior where the version constraints are ignored. This only happens on the py35 build.

I suspect this might be a bug with tox-gh-actions, but I'm unsure. Perhaps there is a way to get the same parallel build behavior without needing that package? I can test stripping it out and see if that fixes anything.

alexweav commented 4 years ago

When tox gets a requires entry that is not installed in the current system, it prefers not to alter the configuration of the system whatsoever. Instead it creates a shim virtualenv which satisfies all the requires, and runs the actual tox suite inside of that, meaning two nested venvs. However, tox-gh-actions does not expect this, and so the config that it injects (describing which tox env to run) is lost to the inner venv, causing the entire test suite to run as if it's running on a dev machine. It then tries to run black, which dies on py35.

On top of that, the FIRST thing that tox suite does is parse the setup.py file, even before installing any of the actual env dependencies listed in tox.ini. Therefore, adding the setuptools constraint to deps in tox.ini, or to install_requires, or to setup_requires, seemingly does nothing, because tox will immediately launch with a setuptools version which cannot handle the setup.py, and immediately fail out. Therefore, the solution seems to be to simply upgrade setuptools on the build machine for py35 before launching anything. That way, tox sees that its requirements are already satisfied, avoids using the shim venv, and launches with the correct version of setuptools anyway.

I ended up adding a pyproject.toml which notates this constraint, so in practice, the generated package will have the proper dependency on setuptools already configured. This means that a user doing an actual install shouldn't run into this, because the generated package will already depend on the correct version of setuptools.

alexweav commented 4 years ago

FYI @DavidCurtiss

DavidCurtiss commented 4 years ago

👍 Looks good.