8000 Ensure nisystemlink-clients-python treats systemlink as an implicit namespace package by alexweav · Pull Request #11 · ni/nisystemlink-clients-python · GitHub
[go: up one dir, main page]
Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from alexweav:fix-namespace-package
Aug 18, 2020
Merged

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

merged 6 commits into from
Aug 18, 2020

Conversation

alexweav
Copy link
Contributor

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
alexweav commented Aug 14, 2020
View reviewed changes
@alexweav
Copy link
Contributor Author

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.

@spanglerco spanglerco self-requested a review August 14, 2020 20:58
@spanglerco spanglerco added the bug Something isn't working label Aug 14, 2020
@DavidCurtiss
Copy link
Contributor

.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
Copy link
Contributor Author
alexweav commented Aug 17, 2020 •
edited
Loading

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
Ensure setuptools is specified in install_requires. Add pyproject.tom…
fd9d2b0 
…l containing build tooling reqs. Keep the requires, and upgrade setuptools during PR build
@alexweav
Copy link
Contributor Author
alexweav commented Aug 17, 2020 •
edited
Loading

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
Copy link
Contributor Author

FYI @DavidCurtiss

@DavidCurtiss
Copy link
Contributor

👍 Looks good.

spanglerco
spanglerco reviewed Aug 17, 2020
View reviewed changes
@alexweav alexweav merged commit a2a0fff into ni:master Aug 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DavidCurtiss DavidCurtiss DavidCurtiss approved these changes

@spanglerco spanglerco spanglerco approved these changes

Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@alexweav @DavidCurtiss @spanglerco
0