-
Notifications
You must be signed in to change notification settings - Fork 436
packaging: introduce pyproject.toml #316
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
Conversation
8128e5d to
555e717
Compare
daviddrysdale
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for looking into this!
Could you also update the end of tools/python/README.md to describe how packaging and PyPI upload works in the new world (and maybe mention any prerequisites, like build)?
cc: @AA-Turner as there was a suggestion you might look into this too.
python/pyproject.toml
Outdated
|
|
||
| [tool.setuptools.packages.find] | ||
| where = ["."] | ||
| include = ["phonenumbers"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to generate packages that omit the metadata, so should this be a list that also includes "phonenumbers.data", "phonenumbers.carrierdata", "phonenumbers.geodata", "phonenumbers.shortdata", "phonenumbers.tzdata" ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From what I tested, the setuptools.packages.find will include packages recursively. If I try to import them, they are available:
$ pip install python/
$ python -c 'from phonenumbers.carrierdata.data0 import data ; print(list(data.items())[:3])'
[('1242357', {'en': 'BaTelCo'}), ('1242359', {'en': 'BaTelCo'}), ('1242375', {'en': 'BaTelCo'})]
$ python -c 'from phonenumbers.geodata.data0 import data ; print(list(data.items())[:3])'
[('1201', {'en': 'New Jersey'}), ('1201200', {'en': 'Jersey City, NJ'}), ('1201216', {'en': 'Jersey City, NJ'})]
$ python -c 'from phonenumbers.shortdata.region_AC import PHONE_METADATA_AC ; print(PHONE_METADATA_AC)'
PhoneMetadata(id='AC', country_code=None, international_prefix=None,
general_desc=PhoneNumberDesc(national_number_pattern='9\\d\\d', possible_length=(3,)),
...
$ python -c 'from phonenumbers.tzdata.data0 import data ; print(list(data.items())[:3])'
[('1', ('America/Adak', 'America/Anchorage', 'America/Anguilla', 'America/Antigua', 'America/Barbados'
...
And on phonenumberslite side, only shortdata is included:
pip install python/phonenumberslite/
$ python -c 'from phonenumbers.geodata.data0 import data ; print(list(data.items())[:3])'
Traceback (most recent call last):
File "<string>", line 1, in <module>
from phonenumbers.geodata.data0 import data
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
ModuleNotFoundError: No module named 'phonenumbers.geodata'
$ python -c 'from phonenumbers.tzdata.data0 import data ; print(list(data.items())[:3])'
Traceback (most recent call last):
File "<string>", line 1, in <module>
from phonenumbers.tzdata.data0 import data
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
ModuleNotFoundError: No module named 'phonenumbers.tzdata'
$ python -c 'from phonenumbers.shortdata.region_AC import PHONE_METADATA_AC ; print(PHONE_METADATA_AC)'
PhoneMetadata(id='AC', country_code=None, international_prefix=None,
general_desc=PhoneNumberDesc(national_number_pattern='9\\d\\d', possible_length=(3,)),
...
setuptools also allows specifying the list of modules to include explicitly, I can do the change if you prefer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's odd, when I ran python -m build with the earlier version of the PR, the .whl and .tar.gz files were missing the metadata, but now it works fine. I wonder what I did differently?
Note: phonenumberslite must now be build from python/phonenumberslite Note: the setup.py is kept for retrocompatibility, in case some script relies on `python setup.py <something-else-than-build-or-install>`
555e717 to
9815c13
Compare
Ah thanks, I missed those instructions. I just updated them. |
python/pyproject.toml
Outdated
|
|
||
| [tool.setuptools.packages.find] | ||
| where = ["."] | ||
| include = ["phonenumbers"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's odd, when I ran python -m build with the earlier version of the PR, the .whl and .tar.gz files were missing the metadata, but now it works fine. I wonder what I did differently?
|
|
||
| [tool.setuptools.packages.find] | ||
| where = ["."] | ||
| include = ["phonenumbers", "phonenumbers.data", "phonenumbers.shortdata"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, if include is recursive, why doesn't the "phonenumbers" here pull in geodata etc?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, it's not recursive.
I was fooled by the *.egg-info directory that seems to be caching some build data from previous builds. Maybe that's what caused the difference of behavior on your system.
I added rm -rf *.egg-info to the build procedure, and fixed the package detection directive to ["phonenumbers*"]
Why: * setuptools would generate warnings about submodules phonenumbers.*
856c44e to
807ec36
Compare
python/pyproject.toml
Outdated
|
|
||
| [tool.setuptools.packages.find] | ||
| where = ["."] | ||
| include = ["phonenumbers*"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This pulls in the files under phonenumbers/pb2/, which weren't previously included in the phonenumbers package.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right. I fixed the package directives to only allow the packages explicitly listed, like in the previous packaging.
21f7afe to
6cadac8
Compare
daviddrysdale
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that there's a couple of differences compared to the old phonenumberslite distribution files (inclusion of pb2/ and tests/*pyi), but the changes are a good thing because they make the lite version more consistent with the main version.
|
Thank you! |
Fixes #307.
Build commands that must be used with

pyproject.toml:(source: packaging.python.org)
Note: phonenumberslite must now be built from
python/phonenumbersliteNote: the
setup.pyis kept for retrocompatibility, in case some script relies onpython setup.py <something-else-than-build-or-install>