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

Improve PyPI packaging #71

Merged
merged 5 commits into from
Oct 11, 2020
Merged

Improve PyPI packaging #71

merged 5 commits into from
Oct 11, 2020

Conversation

gschaffner
Copy link
Contributor

@gschaffner gschaffner commented Oct 1, 2020

Hi! Thanks for the great work on this program.

This draft PR closes #46.

A couple notes:

  • using a src directory. For info about the benefits of this, check out https://blog.ionelmc.ro/2014/05/25/python-packaging/ and setup.py #46 (comment).
  • added a replacement ssh-audit.py wrapper for backwards compatibility
  • for the Python package, changed import sshaudit.sshaudit to import ssh_audit.ssh_audit to adhere to what the vast majority of Python projects with a hyphenated name use. Can revert to sshaudit.sshaudit if backwards compatibility is a concern here.
  • I'm not familiar with snapd so I have not updated the files for snap packaging. These should be fixed before this PR is merged. Feel free to push changes to my fork's branch.
  • windows_build.txt needs checking; I haven't tested the changes I made there.
  • See commit messages :)

And minor notes:

  • The rm in Makefile.pypi seemed overkill to me, so it's a bit weaker now. Can revert the removal of snap cleanup there if you prefer.
  • Made some minor changes that should possibly be moved to a different PR. I included them in this PR to avoid having to rebase the git mv's.
  • The ssh_audit fixture is somewhat unnecessary now and could be removed if you want.
  • using absolute import in __main__.py to work around Relative imports broken in pylint 2.5.2 pylint-dev/pylint#3651

@jtesta
Copy link
Owner

jtesta commented Oct 2, 2020

Thanks for submitting!

I see that your commit messages talk about the tests. On my end, though, both the tox tests and Docker tests are failing. That could be a local problem on my end. Can you verify that they both work for you?

FYI, if you haven't run the Docker tests yet, I'd suggest re-syncing with master first. I just checked in a significant change which makes running the tests much easier (previously, it would try to build the Docker image locally, which is very flaky at the moment; now it pulls my image from Dockerhub).

In effect, this tests that the setup.py configuration is correct.

coverage combine and coverage:paths are added to keep the displayed
coverage paths as src/ssh_audit/*.py instead of
.tox/$envname/**/site-packages/ssh_audit/*.py
Shouldn't need to be an executable.
Related: git has this file tracked as chmod -x.
@gschaffner
Copy link
Contributor Author

I think it's a local problem on your computer. What error are they failing with? Tox passes on my machine and in Travis too.

I haven't run the Docker tests as my Docker install is slightly broken at the moment. If you could run them on this branch that would be great. Not sure when I'll get around to figuring out what broke my install...

I've also rebased this onto the latest master.

@jugmac00
Copy link
Contributor

jugmac00 commented Oct 2, 2020

I can confirm both tox is passing and docker is failing.

❯ ./docker_test.sh 

Starting tests...

Running tests...
Unexpected return value.  Expected: 3; Actual: 0

I am not familiar with the docker tests, but maybe some paths need to be updated? cc @jtesta

Other than that - the PR looks great!

I noticed you deleted the MANIFEST.in file - was that on purpose?

@gschaffner
Copy link
Contributor Author

gschaffner commented Oct 4, 2020

Thanks!

I removed MANIFEST.in in favor of options.package_data in setup.cfg. One less file to have in the repo root. And the license file (formerly listed in MANIFEST.in) is still included in the build via license_file in setup.cfg.

@jtesta jtesta marked this pull request as ready for review October 6, 2020 21:35
@jtesta jtesta merged commit b156649 into jtesta:master Oct 11, 2020
@jtesta
Copy link
Owner

jtesta commented Oct 11, 2020

@gschaffner Thanks for doing this work! It was a big help!

@jtesta
Copy link
Owner

jtesta commented Oct 11, 2020

@jugmac00 And thanks for helping review this PR!

@gschaffner
Copy link
Contributor Author

@jtesta Glad to help!

BTW, it looks like the snap packaging files still need an update. (I had just moved them without modification since I am not familiar with snap.) I'm guessing that you are waiting until after #47 to fix them, but I wanted to bump you to make sure it's on your radar.

@jtesta
Copy link
Owner

jtesta commented Oct 14, 2020 via email

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.

setup.py
3 participants