Skip to content

Fixes #935 - Ensure installed package contents are correct#943

Merged
freakboy3742 merged 14 commits into
beeware:masterfrom
freakboy3742:setup-fix
Jun 12, 2020
Merged

Fixes #935 - Ensure installed package contents are correct#943
freakboy3742 merged 14 commits into
beeware:masterfrom
freakboy3742:setup-fix

Conversation

@freakboy3742
Copy link
Copy Markdown
Member

@freakboy3742 freakboy3742 commented Jun 7, 2020

Modifies the packaging of toga to ensure that test directories are not installed.

The underlying problem was that the include_package_data directive results in the use of MANIFEST.in being used as the source of truth for package data, rather than the package_data directive.

However this is as good a time as any to do some housecleaning around our package configurations, so this PR:

  • Migrates to using new-stye setup.cfg formats (except for the handful of directives - version and test related) that must remain computed.
  • Moves to a single CI workflow, rather than a split CI/build_status configuration.
  • Introduces a tox configuration to verify that package manifests are complete.
  • Introduces a tox configuration to verify that documentation contains no errors.
  • Introduces a tox configuration to run flake8 across the entire codebase; however, this isn't currently included in CI. My intention is to use a subsequent PR to clean up the flake8 problems with the existing codebase, and then enable the tox flake8 test and deprecate beefore.
  • Modifies the toga source distribution to contain a full copy of the examples and source.
  • Removes JavaScript configurations from CI and the Django backend. The Django backend hasn't worked for a long time, and those configurations were mostly causing false positives on security notifications.

PR Checklist:

  • All new features have been tested
  • All new features have been documented
  • I have read the CONTRIBUTING.md file
  • I will abide by the code of conduct

@freakboy3742 freakboy3742 requested review from obulat and saroad2 June 7, 2020 04:22
Copy link
Copy Markdown
Member

@saroad2 saroad2 left a comment

Choose a reason for hiding this comment

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

An important PR. It is good that you move all the content in setup.py to setup.cfg.
I recommend, since you already touch all of this code, to use bump2version in order handle version throughout the code.

If you would like, it can be handed in a different PR.

Comment thread nursery/curses/setup.py
Comment thread nursery/curses/setup.cfg
max-line-length = 119
ignore = E121,E123,E126,E226,E24,E704,W503,W504,C901

[isort]
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do not use isort until it gets to version 5.0.0. It has some major issues that needs to be fixed until then, including consistent handling of 3rd parties libraries in virtual environments. Look at PyCQA/isort#1147

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

That's an interesting bug, to be sure; but I'm not sure it's likely to affect us in practice. It seems to mostly affect packages with... dubiously names (like a top level "config") package.

In practical terms: I've been using isort in production for years without any problems. On top of that, in this particular instance, isort isn't actually hooked into CI. I added the configuration so that we get "black compatible" import styling. This lets us run isort, get imports that are sorted and formatted cleanly, and audit the results before commiting.

In truth, there's a couple of files (like the GTK and Winforms libs module) that we'll need to turn off isort, because they have very specific import ordering. So it's unlikely we're ever going to add isort to CI - the configuration is just here as a convenience.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Perhaps this merits adding a comment next to the [isort] configuration, so that people can quickly get up-to-speed on how we use isort in this project.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I had a lot of problems with that in Eddington. I still hold to my suggestion.

Comment thread release.sh
@paulproteus
Copy link
Copy Markdown
Contributor

I left some comments; they're not showstoppers; feel free to ignore or handle at your leisure.

@freakboy3742 freakboy3742 merged commit 6da8183 into beeware:master Jun 12, 2020
@freakboy3742 freakboy3742 deleted the setup-fix branch June 12, 2020 09:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants