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

Jump to bottom

Cleanup setup.py #281

Merged
merged 4 commits into from May 6, 2015
Merged

Cleanup setup.py #281

merged 4 commits into from May 6, 2015

Conversation

tamird
Copy link
Contributor

@tamird tamird commented Apr 12, 2015

Roughly a combination of #131 and #161 and closes both. @haberman

@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm.

@googlebot
Copy link

CLAs look good, thanks!

@haberman
Copy link
Member

In this project we use 2-space indentation for Python. Would you mind updating the files to reflect this?

@tamird
Copy link
Contributor Author

tamird commented Apr 12, 2015

@haberman done

@tamird
Copy link
Contributor Author

tamird commented Apr 12, 2015

@haberman do you use 4-space indent for continuations?

haberman
haberman reviewed Apr 12, 2015
View reviewed changes
python/setup.py
packages = [ 'google' ],
namespace_packages = [ 'google' ],
test_suite = 'google.protobuf.internal',
# Must list modules explicitly so that we don't install tests.
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't this comment still apply? Seems like we were listing individual modules for a reason.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe that's covered by exclude google/protobuf/internal/*_test.py in MANIFEST.in

@tamird tamird changed the title Cleanup setup.py Cleanup setup.py && get tests passing on python 2.6 Apr 12, 2015
@tamird
Copy link
Contributor Author

tamird commented Apr 12, 2015

@haberman added a few commits to get tests passing under 2.6. PTAL?

@tamird
Copy link
Contributor Author

tamird commented Apr 13, 2015

@haberman this is green; mind having a once-over?

@haberman
Copy link
Member

Any chance this works with Python 2.5?

Also, ideally we'd be able to actually test it under Python 2.5/2.6 using Travis. It's hard to keep things compatible if we are not regularly testing that compatibility. Hopefully this could be added to the Travis build without too much trouble?

@tamird
Copy link
Contributor Author

tamird commented Apr 13, 2015

@haberman I did a lot of googling this morning to work out how to get travis to build a matrix of languages and versions of those languages (currently this project tests with multiple JDKs), and landed on this (still open) issue: travis-ci/travis-ci#2877, so it seems that this isn't possible without quite a bit of work, right now.

I have another commit locally that makes the tests pass under 2.5 - I'll push it now, but it's pretty ugly and way more invasive. Also, the cpp implementation does compile with 2.5, because it uses PyBytes_FromStringAndSize which returns a bytearray, which was added in 2.6.

@tamird
Copy link
Contributor Author

tamird commented Apr 16, 2015

@haberman what are you thinking?

@haberman
Copy link
Member

Sorry for the delay -- I'm trying to nail down whether it's feasible to drop 2.5 support right now by freezing the runtime used by AppEngine's 2.5 runtime.

If we do that, then we can set the minimum supported version to either 2.6 or 2.7 and avoid a lot of pain.

@tamird
Copy link
Contributor Author

tamird commented Apr 16, 2015

Makes sense! Let me know.

@haberman
Copy link
Member

haberman commented May 6, 2015

Ok, we have a green-light to remove Python 2.5 compatibility from this repo!

@haberman
Copy link
Member

haberman commented May 6, 2015

I am inclined to say that we drop support for Python < 2.7. Thoughts?

@tamird
Copy link
Contributor Author

tamird commented May 6, 2015

Yep, I'm in agreement. Want me to update this PR to do that?

tamird added 4 commits May 6, 2015 17:02
@tamird
Don't hard-code the python path
9d9d0b7
@tamird
Whitespace
4f5cbe6
@tamird
[PYTHON] gitignore eggs
6d3e616
@tamird
Noramlize 'setup.py' to fit conventions:
21a7cf9 
- Comply with flake8, except for indentation width, which is 2.

- Move human-centric metadata to top of 'setup()' call.

- Add Trove classifiers for supported Python versions.

- Use 'find_packages()' + MANIFEST.in to avoid errors in listing
  modules and packages.

Closes #131 and #161.
tamird
tamird reviewed May 6, 2015
View reviewed changes
python/setup.py
maintainer_email='protobuf@googlegroups.com',
license='New BSD License',
classifiers=[
'Programming Language :: Python :: 2.7',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this suffices to support only 2.7, right?

@haberman
Copy link
Member

haberman commented May 6, 2015

Sure. If we don't have to worry about Python 2.6, this PR gets a lot shorter/simpler, right?

@tamird
Copy link
Contributor Author

tamird commented May 6, 2015

Yeah, the last two commits are gone

@tamird tamird changed the title Cleanup setup.py && get tests passing on python 2.6 Cleanup setup.py May 6, 2015
@tamird
Copy link
Contributor Author

tamird commented May 6, 2015

Green

@haberman
Copy link
Member

haberman commented May 6, 2015

Thanks so much for this work and for your patience.

If you are able, since you are modifying this part of the code already, could you possibly take a look at this bug and see if it would make sense to fix as part of this PR also?

#333

I'm not too familiar with Python setuptools, so I don't know the right answer here. The main symptom is that when I tried to upload protobuf-python v3.0.0-alpha-2 to PyPI using the command python setup.py sdist bdist_egg upload, the resulting package could not install successfully.

If you'd rather punt on this, that's ok too. I just wanted to mention it now since it seems like a prime time to address it.

@tamird
Copy link
Contributor Author

tamird commented May 6, 2015

I don't know the solution either; let's punt and maybe @tseaver can contribute a fix on top of this PR.

@tamird tamird mentioned this pull request May 6, 2015
Closed
haberman added a commit that referenced this pull request May 6, 2015
@haberman
Merge pull request #281 from tamird/cleanup-setup-py
f47db47 
Cleanup setup.py
@haberman haberman merged commit f47db47 into protocolbuffers:master May 6, 2015
@tamird
Copy link
Contributor Author

tamird commented May 6, 2015

Would you mind closing #161?
On May 6, 2015 19:38, "Joshua Haberman" notifications@github.com wrote:

Merged #281 #281.

—
Reply to this email directly or view it on GitHub
#281 (comment).

@haberman haberman mentioned this pull request May 7, 2015
Closed
@tamird tamird deleted the cleanup-setup-py branch May 7, 2015 00:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@tamird @googlebot @haberman