Skip to content
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.

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

Fix isolated build helper scripts #1629

Merged
merged 1 commit into from
Jul 28, 2020
Merged

Fix isolated build helper scripts #1629

merged 1 commit into from
Jul 28, 2020

Conversation

davidhewitt
Copy link

Fixes #1344

The PEP 517 isolated build uses these helper scripts, but they invoke the __import__ builtin incorrectly.

Copy link
Member

@gaborbernat gaborbernat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.

Copy link
Member

@gaborbernat gaborbernat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requires tests 👍 The changelog is added via towncrier not directly, see example.rst.

@davidhewitt
Copy link
Author

Ah shoot, I'll remove CHANGELOG edit and use towncrier. Also will figure out CI issue.

@@ -4,7 +4,7 @@
backend_spec = sys.argv[2]
backend_obj = sys.argv[3] if len(sys.argv) >= 4 else None

backend = __import__(backend_spec, fromlist=[None])
backend = __import__(backend_spec)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be fromlist=['_trash']

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 - Though also I think the "right" answer on Python 3.3 and above is to use importlib.import_module. So I'll split this into two branches, so that this __import__ branch can be removed once support for old Python versions is removed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I disagree, please just use __import__, no need to involve a separate module to do exactly the same thing

@davidhewitt
Copy link
Author

As for adding tests, I'd appreciate any suggestions on how a testing strategy could be established here.

The "problem case" which this PR fixes is when the backend_spec is just a base package, e.g. no submodule:

>>> __import__('setuptools.build_meta', fromlist=[None])
<module 'setuptools.build_meta' from '...'>

>>> __import__('setuptools', fromlist=[None])
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "<frozen importlib._bootstrap>", line 1033, in _handle_fromlist
TypeError: Item in ``from list'' must be str, not NoneType

I'm not aware of any PEP 517 backends other than maturin which are just a base package and so could be used to write an integration test for this. maturin will be messy to write a test for, because it requires a Rust library to compile.

As such - any advice on suitable testing strategy here?

@gaborbernat
Copy link
Member

gaborbernat commented Jul 26, 2020

For tests you can provide an inline back end and define that to not have that part. There's another pr open for that you could check it out. By the way Ive tried maturin last week and liked it 🙏

@davidhewitt
Copy link
Author

I've reverted to just use __import__ as requested. Thanks for the testing advice - I'll try to find time to write a test as described tomorrow evening.

By the way Ive tried maturin last week and liked it 🙏

\o/ glad to hear!

@davidhewitt
Copy link
Author

Test successfully added and lint fixed. I think this might be good to go?

@gaborbernat
Copy link
Member

applause

@gaborbernat gaborbernat merged commit f6e60fd into tox-dev:master Jul 28, 2020
@gaborbernat
Copy link
Member

@davidhewitt released https://pypi.org/project/tox/3.18.1

@davidhewitt davidhewitt deleted the fix-isolated-build branch July 28, 2020 20:18
ssbarnea pushed a commit to ssbarnea/tox that referenced this pull request Apr 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TypeError: Item in ``from list'' must be str, not NoneType
3 participants