Proper package_data management #196

vishwin · 2022-12-07T00:22:50Z

Turns out data_files was deprecated; PyPA et al recommend that data files be included in the package(s) themselves, in this case, under chirp. Using importlib_resources allows obviating pkg_path() and other hacks about included data files, as they are now fully guaranteed to be present (even in the extremely unlikely load-from-ZIP-file scenario).

(This may be a precursor to full PEP-517 compliance later on)

kk7ds · 2022-12-07T00:26:48Z

Yeah, so this is what I was trying to avoid until we can drop support for the master branch. I initially started with package_resources until I realized that it wasn't going to behave properly until the files are moved. I don't want to move any more files than I have to until we're no longer trying to keep the branches in sync.

As a compromise, what if we made setup.py copy the files into the package before setup() so that we can convince it to put them in the right place (in the package) for just the sdist target?

kk7ds · 2022-12-07T01:01:08Z

setup.py

      version=CHIRP_VERSION,
      url='https://chirp.danplanet.com',
-      python_requires=">=3.3,<4",
-      install_required=['wxPython', 'serial', 'six', 'future'],


This may have been a helpful typo. The reason the test jobs are failing with missing attrdict3 is a known problem with wxPython on Linux only:

wxWidgets/Phoenix#2225

That's why I have the version pins in requirements.txt. My understanding is that you're not supposed to specify versions in install_requires, and only use package names. However, issues like this show how that will never work.

But, even without that problem, it will make us try to compile wxwidgets itself during test environment setup on linux, which would be supremely unfortunate.

The wxPython issue applies to all Unix-like systems (including FreeBSD that I'm using) that did not apply wxWidgets/Phoenix#2224. Versions can absolutely be specified in install_requires, but proceed with caution from a practical point of view.

Yeah, I know it's possible. I'm referring to this recommendation against it:

https://packaging.python.org/en/latest/discussions/install-requires-vs-requirements/#install-requires-vs-requirements-files

It is not considered best practice to use install_requires to pin dependencies to specific versions, or to specify sub-dependencies (i.e. dependencies of your dependencies). This is overly-restrictive, and prevents the user from gaining the benefit of dependency upgrades.

But, even without that problem, it will make us try to compile wxwidgets itself during test environment setup on linux, which would be supremely unfortunate.

The current wxPython versions, ie those not available in the Ubuntu 22.04 LTS Jammy repositories, require wxGTK 3.2, also not available in the repositories. Later versions are only available in Focal. That's the supreme unfortunate part.

I'm not sure what you mean...I have this running locally on jammy (and focal) using distro packages for wxpython.

Jammy (and even Kinetic) only has wxPython 4.0.7, which uses wxWidgets/wxGTK 3.0. wxPython 4.1 uses wx 3.1 (which became 3.2); wxPython 4.2 uses wx 3.2. wxWidgets releases are not API/ABI compatible between one another.

vishwin · 2022-12-07T01:20:17Z

Both pkg_resources and even setup.py are deprecated. grafting arbitrary files/directories into sdists are supported but not for wheel-based bdists, so data files used in the program have to be in the package hierarchy.

tox.ini

requirements.txt

kk7ds

I guess using extras to separate wxpython is reasonable, although it's a bit of an abuse of the "optional functionality" intent.

99% of chirp users consume the bundled builds, and this will break that, so I'll have to spend some time making changes to that stuff and testing it against this.

In the meantime, can you please squash this down to just commits that make logical changes? I'm not a fan of the github-inspired practice of dumping 21 "maybe this will work" commits into the history :) If you're unwilling, I'll do it for you, but I'd appreciate it if you could do it.

I think this is probably two logical changes: 1. Moving the files and 2. changing the requirements stuff.

.github/actions/py3-tox/action.yaml

MANIFEST.in

kk7ds · 2022-12-07T15:26:49Z

chirp/wxui/main.py

+                in importlib_resources.files('chirp.stock_configs').iterdir()
+                if conf.is_file()
+            ]
+        )


I hate black formatting. I just wanted you to know :)

requirements.txt

kk7ds · 2022-12-07T15:31:14Z

setup.py

      version=CHIRP_VERSION,
      url='https://chirp.danplanet.com',
-      python_requires=">=3.3,<4",
-      install_required=['wxPython', 'serial', 'six', 'future'],


I'm not sure what you mean...I have this running locally on jammy (and focal) using distro packages for wxpython.

tox.ini

kk7ds · 2022-12-07T15:34:10Z

tox.ini

@@ -31,7 +32,6 @@ commands =

 [testenv:style]
 basepython = python3
-sitepackages = False


And this? I suspect that we can flip sitepackages=False at the top right?

This was to allow the test to use the Ubuntu-provided wxPython, which would not be in the tox-created virtualenv otherwise.

kk7ds · 2022-12-07T15:37:57Z

setup.py

      entry_points={
          'console_scripts': ["chirp=chirp.wxui:chirpmain"],
      },
+      scripts=['chirpw'],


This should be removed, and the chirpw from manifest. It "works" in that it loads from time to time but things break, I don't want to support it, and plan to remove it from the tree, again, once we're no longer keeping the py2 and py3 branches in sync. Last I checked it didn't even start due to changes sync'd from master. It confuses people having both and I want to avoid that. That also means the gtk extras target can go.

I've been using that since the last commit in the py3 branch in mercurial with only Python syntax changes, using PyGObject3. Even under that (and GTK 3), it is significantly lighter than and has working multi-select and copy-paste functionality compared to the wxPython interface.

It's massively slower in my experience, I'm shocked that you find it faster. There's been over a year of development on the wx ui since the last commit on the mercurial branch. There are still some gaps, but I'm working to close them. If you find any not on the list I sent to the mailing list, feel free to bring them up.

On Windows and macos, wx provides a far superior experience for people (and native widgets), which again, represent 99% of the users :)

MANIFEST.in

vishwin · 2022-12-07T16:07:15Z

Squashing is going to happen eventually, all the separate commits were only for kicking CI and having revert points.

kk7ds · 2022-12-07T16:09:30Z

Squashing is going to happen eventually, all the separate commits were only for kicking CI and having revert points.

Excellent, thanks, I wasn't sure.

kk7ds · 2022-12-08T21:12:05Z

FYI, I'm sorting the locale stuff before getting into this, since that's important (for users) and this is cleanup, but will start on build testing with this soon.

But I'm wondering: do you not want to just go forward with pyproject.toml here instead of fixing setup.py?

vishwin · 2022-12-09T16:56:56Z

PEP-517 compliance is a much bigger beast than just updating setup.py, not least because you will have to majorly change your own later-stage packaging/bundling procedures and documentation (unless you're already having to put more effort in this regard). Was thinking of this as a stopgap before going there.

kk7ds · 2022-12-09T18:59:22Z

PEP-517 compliance is a much bigger beast than just updating setup.py, not least because you will have to majorly change your own later-stage packaging/bundling procedures and documentation (unless you're already having to put more effort in this regard). Was thinking of this as a stopgap before going there.

Yeah, I don't care about PEP-517 I just thought maybe it made sense as setup.py is (for the py3 branch) only used for sdist and that maybe it made sense.

This definitely breaks the packaging stuff, but I'm still working through it and am hopeful I'll be able to get it to work. I just rebased this, included locale in the move, importlib-resources in requirements.txt and will keep working on getting the builds to work based on this.

kk7ds · 2022-12-09T22:16:57Z

@vishwin Okay, I have the bundled builds working with this with the changes I've pushed up here. It's very laborious (windows is slow) to validate, so I'm hoping you're okay with the changes I've made here. Please confirm.

This also doesn't include chirp.locale in the sdist build even though share and stock_configs are, but I'm not sure why and I haven't tried to figure it out. I'm about out of steam today, so maybe you can take a look. If it's only changes to setup.py, I won't have to reverify the other platforms.

If you can confirm this looks okay to you I can merge and get another build generated for the end of the week here. The sdist locale issue probably need not hold us up.

kk7ds · 2022-12-13T03:10:41Z

If you can confirm this looks okay to you I can merge and get another build generated for the end of the week here. The sdist locale issue probably need not hold us up.

@vishwin ?

Data files outside the package are not installed in binary distributions, so move them inside. Use importlib_resources to load icon and stock configs. - use only the newer files(), as_files() and namespace APIs introduced in Python 3.10 - use the importlib_resources package if Python < 3.10 - remove obviated pkg_path() - bump minimum Python version to 3.7 Fix test paths to account for the move

YesNoDialog does not have set_alternative_button_order

setup.py was intentionally typoed to not specify dependencies, but breaks metadata.

wxPython is unnecessary for the automated test suite. Additionally, the GTK interface still works under PyGObject in GTK 3, so re-expose that.

This is required if we're generating a bundled app that uses an executable name that is different than what we want to show up. Specifically for macos, but might apply elsewhere.

kk7ds · 2022-12-14T19:20:23Z

Okay, well, I did a bunch of work to the build system to accommodate this and have been holding up further builds waiting for confirmation. Without that, I'm just going to have to merge this and hope for the best. Not sure the hassle has been worth it in the long run, but hopefully this doesn't cause more trouble.

The default_dir() needs to return a string, but #196 made it return a PosixPath. This manifested as new users unable to open files, but established users could, because last_dir was pulled from the config instead of the platform module.

kk7ds reviewed Dec 7, 2022

View reviewed changes

tox.ini Outdated Show resolved Hide resolved

kk7ds reviewed Dec 7, 2022

View reviewed changes

requirements.txt Outdated Show resolved Hide resolved

vishwin force-pushed the py3-package_data branch 2 times, most recently from 8e11369 to 3482454 Compare December 7, 2022 15:24

kk7ds requested changes Dec 7, 2022

View reviewed changes

vishwin force-pushed the py3-package_data branch 4 times, most recently from eac62d9 to 93b085e Compare December 8, 2022 01:12

kk7ds force-pushed the py3-package_data branch from 93b085e to 07a57a2 Compare December 9, 2022 18:57

kk7ds force-pushed the py3-package_data branch 3 times, most recently from 7351d2a to 928932c Compare December 9, 2022 22:14

kk7ds approved these changes Dec 9, 2022

View reviewed changes

vishwin and others added 6 commits December 14, 2022 11:13

GTK: use importlib_resources and catch up on some refactorings

0b4e614

YesNoDialog does not have set_alternative_button_order

(Attempt to) fix the tests in light of wxPython 4.2.0

b9b88b6

setup.py was intentionally typoed to not specify dependencies, but breaks metadata.

Make wxPython optional for Python packaging purposes

2ca5d1a

wxPython is unnecessary for the automated test suite. Additionally, the GTK interface still works under PyGObject in GTK 3, so re-expose that.

Remove build/

e48ff57

Set wxApp name explicitly

3f001f9

This is required if we're generating a bundled app that uses an executable name that is different than what we want to show up. Specifically for macos, but might apply elsewhere.

kk7ds force-pushed the py3-package_data branch from 928932c to 3f001f9 Compare December 14, 2022 19:13

kk7ds merged commit bcfe5cc into kk7ds:py3 Dec 14, 2022

kk7ds mentioned this pull request Dec 15, 2022

Fix PosixPath returned from default_dir() #251

Merged

vishwin deleted the py3-package_data branch December 27, 2022 21:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Proper package_data management #196

Proper package_data management #196

vishwin commented Dec 7, 2022

kk7ds commented Dec 7, 2022

kk7ds Dec 7, 2022

vishwin Dec 7, 2022

kk7ds Dec 7, 2022

vishwin Dec 7, 2022

kk7ds Dec 7, 2022

vishwin Dec 7, 2022

vishwin commented Dec 7, 2022

kk7ds left a comment

kk7ds Dec 7, 2022

kk7ds Dec 7, 2022

kk7ds Dec 7, 2022

vishwin Dec 7, 2022

kk7ds Dec 7, 2022

vishwin Dec 7, 2022

kk7ds Dec 7, 2022

vishwin commented Dec 7, 2022

kk7ds commented Dec 7, 2022

kk7ds commented Dec 8, 2022

vishwin commented Dec 9, 2022 •

edited

kk7ds commented Dec 9, 2022

kk7ds commented Dec 9, 2022

kk7ds commented Dec 13, 2022

kk7ds commented Dec 14, 2022

Proper package_data management #196

Proper package_data management #196

Conversation

vishwin commented Dec 7, 2022

kk7ds commented Dec 7, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vishwin commented Dec 7, 2022

kk7ds left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vishwin commented Dec 7, 2022

kk7ds commented Dec 7, 2022

kk7ds commented Dec 8, 2022

vishwin commented Dec 9, 2022 • edited

kk7ds commented Dec 9, 2022

kk7ds commented Dec 9, 2022

kk7ds commented Dec 13, 2022

kk7ds commented Dec 14, 2022

vishwin commented Dec 9, 2022 •

edited