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

[proposal] Restructure and publish to PyPI. #28

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

Erotemic
Copy link

@Erotemic Erotemic commented Apr 20, 2020

I found this module through https://askubuntu.com/questions/760896/how-can-i-fix-apt-error-w-target-packages-is-configured-multiple-times

It worked beautifully... when I did a git clone and made a few modifications. The released pyz file did not work by default.

I got

(py38) joncrall@Ooo:~/tmp$     sudo python3 -OEs aptsources-cleanup.pyz
Traceback (most recent call last):
  File "/usr/lib/python3.6/runpy.py", line 193, in _run_module_as_main
    "__main__", mod_spec)
  File "/usr/lib/python3.6/runpy.py", line 85, in _run_code
    exec(code, run_globals)
  File "aptsources-cleanup.pyz/__main__.py", line 12, in <module>
  File "/usr/lib/python3.6/runpy.py", line 208, in run_module
    return _run_code(code, {}, init_globals, run_name, mod_spec)
  File "/usr/lib/python3.6/runpy.py", line 85, in _run_code
    exec(code, run_globals)
  File "aptsources-cleanup.pyz/aptsources_cleanup/__main__.py", line 387, in <module>
  File "aptsources-cleanup.pyz/aptsources_cleanup/__main__.py", line 36, in main
  File "aptsources-cleanup.pyz/aptsources_cleanup/__main__.py", line 202, in parse_args
  File "aptsources-cleanup.pyz/aptsources_cleanup/util/functools.py", line 140, in <lambda>
    op_result = self.__gt__(other)
  File "aptsources-cleanup.pyz/aptsources_cleanup/util/functools.py", line 94, in _get_instance
    # rather than using the corresponding operator.  This avoids possible
  File "aptsources-cleanup.pyz/aptsources_cleanup/util/gettext.py", line 133, in _make_translations
    
  File "aptsources-cleanup.pyz/aptsources_cleanup/util/gettext.py", line 110, in translation
    _c2py_ops = {'||': 'or', '&&': 'and', '/': '//'}
  File "aptsources-cleanup.pyz/aptsources_cleanup/util/zipfile.py", line 38, in open
    
  File "aptsources-cleanup.pyz/aptsources_cleanup/util/zipfile.py", line 27, in getinfo
    crc32 = binascii.crc32
  File "aptsources-cleanup.pyz/aptsources_cleanup/util/zipfile.py", line 71, in _resolve_path
    # we recognize (but not necessarily support) all features up to that version
  File "aptsources-cleanup.pyz/aptsources_cleanup/util/zipfile.py", line 105, in _resolve_path_component
    # indexes of entries in the central directory structure
OSError: [Errno 40] Too many levels of symbolic links: 'aptsources-cleanup.pyz:share/locales/en_US'

It would be nice if I could use the pip package manager to install and run this script.

I'm primarily a Python dev, with a bit of experience in releasing pip installable modules. I'd be willing to help get this module on pypi if you are interested.

The main changes I'd want to make are:

  • move the module from src/... directly to the repo root, as is common practice, as well as get rid of superfluous main modules. The main module can be directly accessed by using python -m <modname>. The command line executable can also be created using the standard setup.py file. I've made these changes already.

  • Create a setup.py file indicating the module requirements (like regex and gitpython) as well as additional info.

  • Either hard code the version in /init.py, or allow setup.py to import the module and get a reasonable version that it can use for publishing. I've currently hard coded a dummy semantic version for myself. I'm not sure if you like semantic versioning or date based versioning, either is fine, but your version auto-generation is currently giving me None values which we don't want setup.py to see when its uploaded to pypi.

EDIT: I just noticed you have a VERSION file, which should work just fine.

  • I haven't added the publish.sh (see https://github.com/Erotemic/ubelt/blob/master/publish.sh) script that I tend to use in my repos when publishing to pypi. Its almost agnostic of the module, so it will be pretty easy to add it if you want that. (also we could setup TravisCI to automatically publish to pypi whenever a commit is pushed to the release branch, we could also set it up to be GPG signed).

  • Update the docs to reflect the new structure. If this proposal is accepted, the new easy way to run this would be:

sudo apt install python3-apt
sudo python3 -m pip install aptsources-cleanup
sudo aptsources-cleanup  # OR sudo python3 -m aptsources-cleanup

This means there wouldn't need to be a curl/wget step when using this module in scripting (This would make sysadmin's lives easier).

  • We could also put in fancy badges (like I have in https://github.com/Erotemic/ubelt and https://github.com/Erotemic/xdoctest) and get CI testing on TravisCI if you're into that.

  • Optional: change tabs to 4 spaces, its driving me crazy, variable tabstops means that code doesn't look the way you intended it to on other's machines, but I'm more than willing to reconvert back to tabs if that's your preference. There are also a few pep8 issues my linter is yelling at me about. In some cases I made style modifications, but I can always revert.

Anyway, thanks for making a helpful tool! Let me know if you want to get this thing on PyPI!

@Erotemic Erotemic changed the title Moved module to root repo directory [proposal] Restructure and publish to PyPI. Apr 20, 2020
@Erotemic
Copy link
Author

I also removed import * in a lot of places, which IMHO makes the code much easier to read. I did this primarily using my mkinit module (Your code actually helped me find an error in the module, which I've been able to fix 😄). This created an explicit list of available functions in the "util" module in util's __init__.py. Thus, in __main__.py I could simply from .util import <name-of-any-function-I-want>. I then looked at unused NameErrors from flake8 to find the names of each function I needed to import from util.

@davidfoerster
Copy link
Owner

davidfoerster commented Apr 20, 2020

The released pyz file did not work by default. I got

(py38) joncrall@Ooo:~/tmp$     sudo python3 -OEs aptsources-cleanup.pyz
Traceback (most recent call last):
  File "/usr/lib/python3.6/runpy.py", line 193, in _run_module_as_main
    "__main__", mod_spec)
  File "/usr/lib/python3.6/runpy.py", line 85, in _run_code
    exec(code, run_globals)
  File "aptsources-cleanup.pyz/__main__.py", line 12, in <module>
  File "/usr/lib/python3.6/runpy.py", line 208, in run_module
    return _run_code(code, {}, init_globals, run_name, mod_spec)
  File "/usr/lib/python3.6/runpy.py", line 85, in _run_code
    exec(code, run_globals)
  File "aptsources-cleanup.pyz/aptsources_cleanup/__main__.py", line 387, in <module>
  File "aptsources-cleanup.pyz/aptsources_cleanup/__main__.py", line 36, in main
  File "aptsources-cleanup.pyz/aptsources_cleanup/__main__.py", line 202, in parse_args
  File "aptsources-cleanup.pyz/aptsources_cleanup/util/functools.py", line 140, in <lambda>
    op_result = self.__gt__(other)
  File "aptsources-cleanup.pyz/aptsources_cleanup/util/functools.py", line 94, in _get_instance
    # rather than using the corresponding operator.  This avoids possible
  File "aptsources-cleanup.pyz/aptsources_cleanup/util/gettext.py", line 133, in _make_translations
    
  File "aptsources-cleanup.pyz/aptsources_cleanup/util/gettext.py", line 110, in translation
    _c2py_ops = {'||': 'or', '&&': 'and', '/': '//'}
  File "aptsources-cleanup.pyz/aptsources_cleanup/util/zipfile.py", line 38, in open
    
  File "aptsources-cleanup.pyz/aptsources_cleanup/util/zipfile.py", line 27, in getinfo
    crc32 = binascii.crc32
  File "aptsources-cleanup.pyz/aptsources_cleanup/util/zipfile.py", line 71, in _resolve_path
    # we recognize (but not necessarily support) all features up to that version
  File "aptsources-cleanup.pyz/aptsources_cleanup/util/zipfile.py", line 105, in _resolve_path_component
    # indexes of entries in the central directory structure
OSError: [Errno 40] Too many levels of symbolic links: 'aptsources-cleanup.pyz:share/locales/en_US'

Thanks for the bug report. I forgot a not in a code change and neglected to test the Python archive. Fixed in 7fb44b8 (v0.1.7.2).


It would be nice if I could use the pip package manager to install and run this script.

I thought about that and decided against it because I’m unsure to what extent the only hard dependency, python-apt, is tied to a particular version of the underlying C library which differs between distros and/or releases. Thus, I prefer to ask users to install it manually via Apt than to auto-install a particular version via Pip. I even go as far as to ask users to run Python with the -Es flags to avoid libraries from sources other than the Apt package manager as much as possible.

Additionally, I suspect that most users of the target demographic don’t have Pip installed on their machines; asking them to

apt install python3-pip
pip3 install aptsources_cleanup

instead of

wget https://github.com/davidfoerster/aptsources-cleanup/releases/download/.../aptsources-cleanup.pyz
apt install python3-apt

provides no benefit in my opinion.

I'm primarily a Python dev, with a bit of experience in releasing pip installable modules. I'd be willing to help get this module on pypi if you are interested.

Thanks for your offer! I’m willing to look into Pip packaging and publishing as long as my personal additional effort stays within reason compared to the expected (user) benefit. :-] Although a Debian package would be even more helpful. ;-D


The main changes I'd want to make are:

  • move the module from src/... directly to the repo root, as is common practice, as well as get rid of superfluous main modules. The main module can be directly accessed by using python -m <modname>. The command line executable can also be created using the standard setup.py file. I've made these changes already.

Yes, the __main__.py entry point is purely a requirement of the Python archive format.

  • Create a setup.py file indicating the module requirements (like regex and gitpython) as well as additional info.

Both modules are very optional:

  • regex only matters for some language scripts (Devanagari being the most common) – maybe – and, so far, nobody submitted a translation for those. For everything else the built-in re module is just as good.
  • git is only relevant for people who cloned the repository and even then of small additional benefit.
  • I haven't added the publish.sh (see https://github.com/Erotemic/ubelt/blob/master/publish.sh) script that I tend to use in my repos when publishing to pypi. Its almost agnostic of the module, so it will be pretty easy to add it if you want that. (also we could setup TravisCI to automatically publish to pypi whenever a commit is pushed to the release branch, we could also set it up to be GPG signed).

Sounds good.

  • Update the docs to reflect the new structure. If this proposal is accepted, the new easy way to run this would be:
sudo apt install python3-apt
sudo python3 -m pip install aptsources-cleanup
sudo aptsources-cleanup  # OR sudo python3 -m aptsources-cleanup

This means there wouldn't need to be a curl/wget step when using this module in scripting (This would make sysadmin's lives easier).

Experienced sys admins and automated execution aren’t the target group and aim of this application! It’s meant for one-time or occasional, deliberate, manual use. If a system set-up frequently leads to duplicate Apt source entries one should solve the underlying cause for that. If a sys admin frequently adds duplicate Apt source entries manually they should stop doing that.


  • Optional: change tabs to 4 spaces, its driving me crazy, variable tabstops means that code doesn't look the way you intended it to on other's machines, but I'm more than willing to reconvert back to tabs if that's your preference. There are also a few pep8 issues my linter is yelling at me about. In some cases I made style modifications, but I can always revert.

Yes, I know Guido said we should use exactly 4 spaces per indentation level and not tab stops. I’m also aware that a uniform source code format within one project or group has huge benefits. I simply disagree that those sets are made up of all pieces of Python code and all Python developers out there.

The current indentation scheme is meant to work for all tab length ≥ 2 with resulting line lengths being the only other noticeable change. You’re welcome to set it to 4 or whatever length you prefer your editor.

If more people end up contributing code I wouldn’t be opposed to closer adherence to PEP8. One semi-regular contributor is probably enough to tip my scales.

Anyway, thanks for making a helpful tool! Let me know if you want to get this thing on PyPI!

Appreciated.

@Erotemic
Copy link
Author

Erotemic commented Apr 26, 2020

So, I've been able to restructure this PR such that it doesn't create much of a diff with the base directory. While I would love to run autopep8 on the whole thing and debate code style religious ideology 😄, that's best left for a separate PR. In this one I'm more concerned about having a distribution method that doesn't require memorizing a url.

I've taken care to ensure the pyz distribution still works. This means I kept the src directory and the __main__ file src is still around.

The one change I made to the actual source code was in utils/pkg.py the FileDescriptor.__enter__ seemed to return an integer error code instead of raising an exception in some cases. I've fixed this such that is now raises an EnvironmentError.

Other than that, this PR only contains new files. The setup.py is the most important of these files. This allows you to clone the repo and pip install . or without cloning pip install git+https://github.com/davidfoerster/aptsources-cleanup.git. This installs the aptsources-cleanup CLI script to the prefix bin directory. It also creates the distribution files that can be uploaded to pypi.

The other scripts I've added are publish.sh and .travis.yml. These can be used together to automatically publish new versions to pypi whenever you push to the "release" branch (you can change that to any branch). But to set these up it does requires some work on your part. I'm doing my best to minimize the work you need to do.

Manual Publishing

At the minimum you will need to create a username and password on pypi. To manually publish to pypi, you can then simply run:

    cd <YOUR REPO>

    # Set your variables or load your secrets
    export TWINE_USERNAME=<pypi-username>
    export TWINE_PASSWORD=<pypi-password>

    TAG_AND_UPLOAD=yes DEPLOY_BRANCH=master ./publish.sh

(modifying values as appropriate).

This assumes you don't want to sign with a GPG key. If you want to do that, then set assuming you have GPG keys setup, set USE_GPG=True and GPG_KEYID=.

For reference I have notes on how to setup GPG in another PR where I helped a different repo go through a similar process.

Automatic Publishing

While manual publishing is simpler in the short-term, I prefer to automatically publish my modules whenever I push to the release branch. However, the setup is a bit more involved. There are instructions on how to this in the .travis.yml file, but I can guide you through with specific instructions if you are interested. The gist is that you will need to setup several encrypted secrets so TravisCI can safely use them.

So, what I need to know is

  1. Do you want to sign your pypi release with GPG keys? Or would you rather release an unsigned package? (Note: pypi doesn't actually do any checking against these signatures, but it does host them. Hopefully in the future it will actually check them)

  2. Do you want to publish manually via the publish script (alternatively you can simply use setup.py and twine and follow an online tutorial), or would you rather publish via TravisCI?

Comment on lines +23 to +25
if isinstance(md5sums_fd, int):
raise EnvironmentError(
'Got error code {} when opening {}'.format(md5sums_fd, md5sums_file))
Copy link
Owner

@davidfoerster davidfoerster Apr 27, 2020

Choose a reason for hiding this comment

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

Thanks for bringing that bug to my attention! Fixed in f397f3a which obviates the need for this patch here.

@ghost
Copy link

ghost commented Sep 22, 2020

amazing. thank you for writing this

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