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

Reorganize source directory #279

Merged
merged 12 commits into from
Nov 1, 2020
Merged

Reorganize source directory #279

merged 12 commits into from
Nov 1, 2020

Conversation

gnarlyquack
Copy link

Description

This series of commits reorganizes the source directory in accordance with the principles discussed here: https://blog.ionelmc.ro/2014/05/25/python-packaging/. As a consequence, maintenance and installation of the project should be somewhat streamlined and simpler.

How Has This Been Tested?

The application runs, the test suite passes, and all github workflows succeed.

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.

Let's ensure topic branches are in a usable state before pull requests
are made
Use setuptools.find_packages to avoid manually maintaining the package
list.

This restores __init__.py to the gourmet/exporters directory so it can
be discovered.

This also eliminates a bug in the previous crawl_plugins function that
erroneously discovered __pycache__ and other non-source directories.
We probably don't want to include tests when building non-source
distributions, and non-development installs may not have the
dependencies installed to run them anyway.
This can cause installation to fail if we end up trying to import a
dependency that isn't (yet) installed.
Generally the project root will be our current working directory, which
Python automatically includes in its system path. Since gourmet is no
longer in the project root, we must now explicitly install it to run it,
which helps ensure we have a working, installable distribution.
We can now just "graft" the src directory, which means any additional
package data will be automatically included.
Gourmet can now be installed in one step and plugin-specific
dependencies can be optionally installed using 'extras_require'.

setuptools is removed as a requirement since this provides the build
system and must already be installed for anything to work.
A development install can now be installed in one step using
'development.in'. This installs gourmet in "editable" mode as well as
additional development dependencies.

'development.txt' is renamed to 'development.in' to support pinning of
dependencies (using, e.g., pip-tools). Unfortunately, this only works
for local installs since gourmet is a "local" package and results in a
non-portable, absolute file path being generated in 'development.txt'.
Consequently, 'development.txt' is added to .gitignore, and build
environments can just use 'development.in' directly.

wheel is removed as a development dependency since it must already be
installed in order for pip to build wheels when installing dependencies.
Copy link
Collaborator

@cydanil cydanil left a comment

Choose a reason for hiding this comment

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

Nice rework of the setup.py.

I generally approve of the changes, they're good improvements. But please next time break this down into smaller pull requests: such large PRs make them hard to review.

For example, the restructuring, the rework of setup.py, changes to the CI behaviour, rewriting of the CONTRIBUTING.md, and removal of unused imports could be a way to separate the different aspects of this PR.

Lastly, did you create a working flatpak from this branch?

.github/workflows/build.yml Outdated Show resolved Hide resolved
.github/workflows/tests.yml Outdated Show resolved Hide resolved
.github/workflows/tests.yml Show resolved Hide resolved
.github/workflows/tests.yml Show resolved Hide resolved
CONTRIBUTING.md Outdated

Ensure your system has the necessary prerequisites installed:
- [Python](https://www.python.org/), which is what Gourmet is written in. Only
currently-supported versions of Python 3 are supported.
Copy link
Collaborator

Choose a reason for hiding this comment

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

There's been a discussion previously, where we agreed to follow Ubuntu LTS versions of Python.

Copy link
Author

@gnarlyquack gnarlyquack Nov 1, 2020

Choose a reason for hiding this comment

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

I've updated the documentation, but since Debian-based distros pin their software versions with every release, this really shouldn't be an issue. Assuming Gourmet gets included in the Ubuntu repositories, it will be whatever version is current at the time the release is frozen, which will presumably correspond with a version of Python that is supported at the time. IMO, the question is more: do you want to support old versions of Gourmet that may be in current (LTS) versions of Ubuntu (or Debian, or Mint, etc.)?

@@ -11,10 +11,13 @@ gourmet.pot
gourmet.appdata.xml
gourmet.desktop
po/.intltool-merge-cache
/development.txt
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's with this file? I understand that it was removed for development.in

Copy link
Author

Choose a reason for hiding this comment

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

As noted in the commit message, if I use something like pip-tools to pin dependencies, development.txt will end up with a non-portable list of dependencies. Something like (note the absolute path on the first line):

#
# This file is autogenerated by pip-compile
# To update, run:
#
#    pip-compile development.in
#
-e file:///home/gnarlyquack/Code/gourmet/code/kirienko  # via -r development.in
argcomplete==1.12.1       # via gourmet
attrs==20.2.0             # via pytest
beautifulsoup4==4.9.3     # via gourmet, mf2py
certifi==2020.6.20        # via requests
cffi==1.14.3              # via cryptography
chardet==3.0.4            # via requests
cryptography==3.2.1       # via secretstorage
decorator==4.4.2          # via validators
dogtail==0.9.10           # via -r development.in
ebooklib==0.17.1          # via gourmet
extruct==0.10.0           # via scrape-schema-recipe
flake8==3.8.4             # via -r development.in
html-text==0.5.2          # via extruct
html5lib==1.1             # via mf2py
idna==2.10                # via requests
importlib-resources==3.3.0  # via scrape-schema-recipe
iniconfig==1.1.1          # via pytest
isodate==0.6.0            # via rdflib, scrape-schema-recipe
jeepney==0.4.3            # via keyring, secretstorage
jstyleson==0.0.2          # via extruct
keyring==21.4.0           # via gourmet
lxml==4.6.1               # via ebooklib, extruct, gourmet, html-text
mccabe==0.6.1             # via flake8
mf2py==1.1.2              # via extruct
mypy-extensions==0.4.3    # via mypy
mypy==0.790               # via -r development.in
packaging==20.4           # via pytest
pillow==8.0.1             # via gourmet, reportlab
pluggy==0.13.1            # via pytest
py==1.9.0                 # via pytest
pycairo==1.20.0           # via pygobject
pycodestyle==2.6.0        # via flake8
pycparser==2.20           # via cffi
pyenchant==3.1.1          # via gourmet, pygtkspellcheck
pyflakes==2.2.0           # via flake8
pygobject==3.38.0         # via gourmet
pygtkspellcheck==4.0.5    # via gourmet
pyparsing==2.4.7          # via packaging, rdflib
pytest==6.1.2             # via -r development.in
rdflib-jsonld==0.5.0      # via extruct
rdflib==4.2.2             # via extruct, rdflib-jsonld
reportlab==3.5.55         # via gourmet
requests==2.24.0          # via gourmet, mf2py, scrape-schema-recipe
scrape-schema-recipe==0.1.3  # via gourmet
secretstorage==3.1.2      # via keyring
selenium==3.141.0         # via gourmet
six==1.15.0               # via cryptography, ebooklib, extruct, html5lib, isodate, packaging, validators, w3lib
soupsieve==2.0.1          # via beautifulsoup4
sqlalchemy==1.3.20        # via gourmet
toml==0.10.2              # via gourmet, pytest
typed-ast==1.4.1          # via mypy
typing-extensions==3.7.4.3  # via mypy
urllib3==1.25.11          # via requests, selenium
validators==0.18.1        # via scrape-schema-recipe
w3lib==1.22.0             # via extruct
webencodings==0.5.1       # via html5lib

As a consequence, I want to ensure this file doesn't end up in the repository.

Although I would very much like to include pinned dependencies in the repository (at least for development), this has been an open issue for years, and there isn't a solution that doesn't make the installation process on our end more complicated (this is also relevant).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for the explanations

@@ -0,0 +1,9 @@
graft src
graft tests
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since the tests are now apart from the source, there's no need to package them, no?

Copy link
Author

Choose a reason for hiding this comment

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

They'll still be included in source distributions but not built distributions (e.g., wheels), which is what I think we want.

development.in Outdated Show resolved Hide resolved
@gnarlyquack
Copy link
Author

Lastly, did you create a working flatpak from this branch?

Yes. I don't use flatpaks though, so someone else will have to download and test the artifact.

Copy link
Collaborator

@cydanil cydanil left a comment

Choose a reason for hiding this comment

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

LGTM! This PR brings a well needed overhaul of the project. Thanks for taking the time to work on this :)

@@ -8,6 +8,7 @@ on:
branches: [master]
pull_request:
branches: [master]
workflow_dispatch:
Copy link
Collaborator

Choose a reason for hiding this comment

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

okay

@@ -11,10 +11,13 @@ gourmet.pot
gourmet.appdata.xml
gourmet.desktop
po/.intltool-merge-cache
/development.txt
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for the explanations

@cydanil cydanil merged commit 052fa7a into kirienko:master Nov 1, 2020
@gnarlyquack gnarlyquack deleted the reorganize branch November 2, 2020 20: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.

2 participants