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

Use pbr for packaging python module and clean up setup.py #5

Closed
wants to merge 15 commits into from

Conversation

Projects
None yet
2 participants
@copyninja
Copy link
Contributor

commented May 16, 2017

Use pbr (Python build reasonableness) for managing the setuptools packaging. I've mimicked most functionality in existing setup.py except the extra_requires. extra_requires points to a hash which I'm not clear is really used or just a left over.

I would be happy to get a review on my changes.

@copyninja

This comment has been minimized.

Copy link
Contributor Author

commented May 21, 2017

I will reopen the PR once I fix travis error.

@copyninja copyninja closed this May 21, 2017

copyninja added some commits May 21, 2017

Drop universal wheel as per pbr recommendation
We build binary extension where shipping universal wheel is not
recommended.

@copyninja copyninja reopened this May 21, 2017

@copyninja

This comment has been minimized.

Copy link
Contributor Author

commented May 21, 2017

@vu3rdd I would be happy if you can have a look at the changes and provide some feedback.

@warner

This comment has been minimized.

Copy link
Member

commented Jul 16, 2017

Hey, this is a great direction to go. Could you try to address a few problems?:

  • pull in the changes I made (on top of your branch) in https://github.com/warner/zfec/tree/pr5 , which fixes a bunch of typos (the metadata changed in your branch, which probably wasn't your intention), fixes the --stride= argument, and removes the old setup.py/setup.cfg files (which are still in git history if we ever need them).
  • this switch to PBR causes the computed version string to break (try a setup.py --version: it should be 1.4.24-something, but it comes out as 0.0.1-something). We were using Versioneer before, which looked for a git tag with a zfec- prefix, and I'm guessing PBR can't handle tags with prefixes. Is there any way we can keep using Versioneer?
  • I'd love for the standard test command to be just "run tox". I think this will still accomplish that, but could you change the .travis.yml to use tox too? In particular I'd like to see that pip install -r requirements.txt removed from .travis.yml, if that's not what we'd advise devs to use. Tox should install the deps itself (or, rather, tox should pip install -e ., and pip should get the deps, via install_requires=)
  • running setup.py appears to modify ./changelog, which isn't a great thing for a build step to do. I think it also created an AUTHORS file which wasn't there before.
  • the .eggs/ directory needs to be added to .gitignore, if it's going to be populated through the setup_requires=

thanks!

copyninja added some commits Jul 16, 2017

Use versioneer to provide version output
pbr does not look at tags with zfec- prefix and hence we get weird
version number when we run

       python setup.py --version

We follow pbr's approach of overriding version by setting output from
`versioneer.get_version` to environment variable PBR_VERSION. versioneer
section is added to setup.cfg providing required configuration values.
Move installing of requirements.txt to tox
Also invoke codecov (not sure if that works fine).
@copyninja

This comment has been minimized.

Copy link
Contributor Author

commented Jul 16, 2017

@warner Thanks for the review and support in IRC :-). I've done all the changes you asked for. Please let me know if you think some more changes needed for the same.
And yeah sorry about the silly typo's which you had to fix :-).

@warner

This comment has been minimized.

Copy link
Member

commented Jul 17, 2017

No worries :)

In tox.ini, I notices that you're using deps = --editable=., which seems like a bit of a hack. Doesn't Tox have some easier way to express that, like develop = true or something?

@copyninja

This comment has been minimized.

Copy link
Contributor Author

commented Jul 17, 2017

@copyninja

This comment has been minimized.

Copy link
Contributor Author

commented Jul 17, 2017

@warner I removed deps and added usedevelop and it does the same as previous deps=--editable=.. Let me know if anything else needs to be tuned.

@warner

This comment has been minimized.

Copy link
Member

commented Feb 2, 2018

I've rebased this to current master (the recent py3 support caused quite a few conflicts), and squashed it to a single commit (to make the merge easier). I'll land this new branch momentarily. Thanks!

@warner warner closed this in 9d4c465 Feb 2, 2018

@copyninja copyninja deleted the copyninja:pbr branch Jun 2, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.