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

Add setup.py script to build the package using setuptools #143

Merged
merged 1 commit into from
Jun 12, 2020
Merged

Add setup.py script to build the package using setuptools #143

merged 1 commit into from
Jun 12, 2020

Conversation

jdufresne
Copy link
Contributor

Setuptools is a fully-featured, actively-maintained, and stable library
designed to facilitate packaging Python projects. It is the standard
library used to create Python packages by the community.

The setup.py script allows building an sdist package and a wheel
package. Installing the package with setuptools installs a gitimerge.py
module and a git-imerge command line script.

As this package is pure Python and compatible with both Python 2 & 3,
the wheel is marked as universal.


This continues the work started in PR #55. If work in that PR continues, I'm happy to close this one.

@mhagger
Copy link
Owner

mhagger commented May 27, 2019

Thanks so much for the PR. I'm obviously no expert in setuptools (otherwise I'd have set this up myself) but it basically looks plausible to me.

But I think a couple of things are needed before this can be merged:

  • Use/install instructions (e.g., in the README file). Before this PR, you could clone this repo and run git-imerge from the main directory without any extra steps. But since you have renamed the main file, that is no longer possible. What would be the simplest way for a user to get started using git-imerge if this PR is merged?

  • The tests (such as they are) no longer work. Currently, you can clone the repo then run

      t/create-test-repo
      t/test-unconflicted
      t/test-conflicted
      t/test-drop
    

    to exercise the code. (I realize that this isn't documented.) That doesn't work anymore because of the renaming mentioned above. How do the tests have to be modified to work again?

  • Does setuptools offer any help for installing the bash completion script, git-imerge.bashcomplete?

@jdufresne
Copy link
Contributor Author

Thanks for the feedback. I'm happy to work through this and then I'll post a message here when complete.

Use/install instructions (e.g., in the README file).

Should I plan instructions to install the via PyPI? IMO, that would be most convenient to end users. If you're unfamiliar with the steps to upload to PyPI, I can help you through them.

@jdufresne
Copy link
Contributor Author

I've updated the PR with the following changes:

  • Added installation and testing instructions to README
  • Added bash completion support to the setup.py script through the data_files feature
  • Added tox to automate testing
  • Removed the Makefile install target, as now setup.py should be used to install the package.

Thanks! Let me know if you have any additional feedback.

Copy link
Owner

@mhagger mhagger left a comment

Choose a reason for hiding this comment

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

That's awesome, @jdufresne! I left a little comment about some duplicated code, but otherwise it looks good to me (to the extent that I can judge).

Should I plan instructions to install the via PyPI? IMO, that would be most convenient to end users. If you're unfamiliar with the steps to upload to PyPI, I can help you through them.

I actually have a PyPI account ("mhagger") and have uploaded other projects there before…like ten years ago 😕 So if you could give me a two-sentence reminder or a pointer to instructions, that would be helpful. I assume that git-imerge would be a suitable project name? (It's not yet claimed.)

Also, I suppose that I should release a version 1.2.0 while I'm at it, so that the release agrees with the packaging changes that you are making.

if __name__ == '__main__':
try:
main(sys.argv[1:])
except Failure as e:
sys.exit(str(e))


if __name__ == "__main__":
Copy link
Owner

Choose a reason for hiding this comment

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

This block and the if __name__ == '__main__': block just above it seem to be redundant. And this one doesn't appear to pass the command-line arguments into main().

include t/create-test-repo
include t/test-conflicted
include t/test-drop
include t/test-unconflicted
Copy link
Owner

Choose a reason for hiding this comment

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

Just to be clear...the files under t/ are only needed for testing. Their inclusion here doesn't imply that they will be installed for end-users, does it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct.

It is customary to include test files in an sdist (source distribution).

  1. Many Linux distro packagers require tests and for them to pass when adding a new package to their repository.
  2. It allows end users to sanity check the state of the released package.

While these files are included in the .tar.gz file, they are not installed on the user's system.

@mhagger
Copy link
Owner

mhagger commented Jun 3, 2019

Oh, another thing: it looks like tox spews a bunch of files under /.tox. That directory should probably be added to .gitignore.

@jdufresne
Copy link
Contributor Author

jdufresne commented Jun 4, 2019

So if you could give me a two-sentence reminder or a pointer to instructions, that would be helpful. I assume that git-imerge would be a suitable project name? (It's not yet claimed.)

Sure thing.

I think git-imerge would be a great name!

Once you've finalized all changes to the release, you normally do something like:

$ python3 setup.py sdist
$ twine upload dist/*

The first command builds the sdist file (source distribution). The second one uploads it. It will prompt you for a username and password. If you don't have twine installed, you can install it using pip:

$ pip install twine

If you're using Linux, it may also be available through your distro repository.

You can also execute twine with the command:

$ python3 -m twine upload dist/*

The following guide is an excellent source for more details:

https://packaging.python.org/tutorials/packaging-projects/

@jdufresne
Copy link
Contributor Author

jdufresne commented Jun 4, 2019

Changes:

  • Cleaned up duplicate __main__ block
  • Added .tox/ to .gitignore
  • Included tox.ini in MANIFEST.in to facilitate testing

gitimerge.py Outdated
try:
main(sys.argv[1:])
main()
Copy link
Owner

Choose a reason for hiding this comment

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

It took me a while to figure out how the command-line arguments are getting through to main. (I wondered about that here.) It used to be that they were passed to main here. But now, nothing is passed to main, and nothing anywhere in the script references sys.argv anymore.

It turns out that we are relying on this call to parser.parse_args(), which now passes None to that method, which triggers argparse's default behavior of taking the arguments directly from sys.argv.

This seems a bit magical to me. Is there a reason not to continue passing sys.argv[1:] explicitly here, as was done before? ("This is the standard pattern now" would an acceptable answer.) Otherwise, could main get a short docstring explaining that args = None implies that the arguments will be taken from sys.argv?

Otherwise, everything looks great! Thanks for your patience 😃

Setuptools is a fully-featured, actively-maintained, and stable library
designed to facilitate packaging Python projects. It is the standard
library used to create Python packages by the community.

The setup.py script allows building an sdist package and a wheel
package. Installing the package with setuptools installs a gitimerge.py
module and a git-imerge command line script.

A tox configuration was added to automate testing in a virtualenv.

The README has been updated with new installation and testing instructions.
@jdufresne
Copy link
Contributor Author

I have restored the user of sys.argv[1:].

I'm ok either way. I only removed it because it feels rather typical to me across argparse uses I've seen.

@mhagger
Copy link
Owner

mhagger commented Jun 13, 2019

I just pushed a couple more commits to a branch "setuptools" under mhagger/git-imerge. If they look good to you, I'll merge that branch, then in a separate commit bump the version number to 1.2.0, create a tag, then push a release to PyPI. Thanks again for your patience.

@jdufresne
Copy link
Contributor Author

Yup, those all look good to me. Thanks!

@abitrolly
Copy link
Contributor

abitrolly commented Nov 30, 2019

The ability to run git-imerge directly from git clone is very convenient. https://github.com/mhagger/git-imerge/tree/setuptools loses that ability.

There are also new attempts to package Python code without setup.py - see https://github.com/takluyver/flit

@pmav99
Copy link

pmav99 commented Feb 22, 2020

I just pushed a couple more commits to a branch "setuptools" under mhagger/git-imerge. If they look good to you, I'll merge that branch, then in a separate commit bump the version number to 1.2.0, create a tag, then push a release to PyPI. Thanks again for your patience.

@mhagger any updates?

@mhagger mhagger merged commit 85812fc into mhagger:master Jun 12, 2020
@mhagger
Copy link
Owner

mhagger commented Jun 12, 2020

I don't know why I never merged this. Sorry.

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.

4 participants