Moving core into designated package. #2367

dhermes · 2016-09-20T20:48:20Z

This is a tiny chunk of #2357 that should be easier to review and discuss.

Still missing (per discussion with @jonparrott):

Putting unit tests into the core subdirectory
A better story for isolating packages / making sure the proper dependencies are listed (may have to have core/tox.ini and run that separately, though this will become too slow, so maybe we'll switch to using nox)

NOTE: Has #2394 as diffbase.

core/setup.py

@@ -0,0 +1,58 @@
+import os


core/setup.py

+
+PACKAGE_ROOT = os.path.abspath(os.path.dirname(__file__))
+
+with open(os.path.join(PACKAGE_ROOT, 'README.rst')) as file_obj:


core/setup.py

+REQUIREMENTS = [
+    'httplib2 >= 0.9.1',
+    'googleapis-common-protos',
+    'oauth2client >= 2.0.1',


theacodes · 2016-09-20T21:06:36Z

@dhermes do you want to try to address those still missing items in this PR or another?

dhermes · 2016-09-20T23:11:02Z

do you want to try to address those still missing items in this PR or another?

I'd like to iron out the strategy during this PR and I'd love to hear from @tseaver.

dhermes · 2016-09-20T23:12:57Z

FYI I folded the license header change and the upper bound for oauth2client into the original commit.

theacodes · 2016-09-20T23:14:59Z

For the tests, I would suggest:

Move them into the core folder under tests
Be sure to update the manifest.in and such.
Make a core/tox.ini for the time being.

Even if you decide to use nox to automate across the packages, you can still use tox within each package. The sample I sent you earlier does just that (nox is top-level, but nox just shells out to tox for each sub-folder).

dhermes · 2016-09-20T23:51:59Z

D'oh! Forgot MANIFEST.in!

dhermes · 2016-09-21T04:14:55Z

@tseaver @jonparrott Isolating core with its own tox.ini has revealed that our user agent code has to change from what it is today. Right now it depends on the version (via get_distribution) of google-cloud. This is problematic, since we are going to a future where users don't even need to have the umbrella package installed.

dhermes · 2016-09-21T05:14:44Z

PTAL. I created a tox config in the core subdirectory.

No longer true:

~~Though right now tox -e cover is failing on the umbrella because it isn't picking up the moved unit tests.~~

~~@jonparrott Any suggestions for how to pick them up?~~

.travis.yml

  - tox -e lint
  - tox -e cover
-  - cd core && tox -e cover
+  - (cd core && tox -e cover)


core/MANIFEST.in

@@ -0,0 +1,3 @@
+include README.rst
+graft google
+global-exclude *.pyc


tseaver

I'm really uncomfortable with where the core subdirectory change is going: it is causing us to replicate files / testing logic, etc. all over the place.

Could we avoid that, and have separate files in the root, instead? E.g. setup_core.py, tox-core.ini, README-core.py, etc.

.coveragerc

@@ -6,8 +6,6 @@ omit =
    */_generated/*.py
    # Packages in the "google.cloud" package that we don't own.
    */google/cloud/gapic/*
-fail_under = 100
-show_missing = True


.travis.yml

  - tox -e lint
  - tox -e cover
+  - (cd core && tox -e cover)


core/google/cloud/connection.py

@@ -28,7 +28,7 @@
 """The base of the API call URL."""

 DEFAULT_USER_AGENT = 'gcloud-python/{0}'.format(
-    get_distribution('google-cloud').version)
+    get_distribution('google-cloud-core').version)


theacodes · 2016-09-21T21:21:02Z

Could we avoid that, and have separate files in the root, instead? E.g. setup_core.py, tox-core.ini, README-core.py, etc.

@tseaver how does that avoid duplication? That only makes things more complicated because the package-specific setup.py and manifest will have to go out of its way to exclude the other packages. It also breaks the convention of python setup.py install and pip install .

dhermes · 2016-09-22T16:29:17Z

@tseaver Regarding duplication, these are the things right now that are being duplicated

MANIFEST.in
Majority of setup.py
Some __init__.py and directory structure (both in unit_tests/ and in google/)
unit_tests/_testing.py is mostly duplicated, though as of right now, _Monkey is the only piece used by the core subpackage and mock.patch() can handle _Monkey's job just fine

The setup.py and __init__.py / structure duplication are unavoidable. However, the rest aren't really duplication at a certain point, e.g. MANIFEST.in may very well contain different things for each subpackage.

As far as the isolation, you even see with some of the commits in this PR that putting core on its own revealed some missing code paths in the core unit tests.

This way we can be sure what each subpackage depends on and what needs to be tested for users that don't install the umbrella package.

scripts/run_pylint.py

@@ -37,6 +37,8 @@
 ]
 IGNORED_FILES = [
    os.path.join('docs', 'conf.py'),
+    os.path.join('google', '__init__.py'),
+    os.path.join('google', 'cloud', '__init__.py'),


core/setup.py

+
+setup(
+    name='google-cloud-core',
+    version='0.19.0',


core/setup.py

+        'Programming Language :: Python :: 3.5',
+        'Topic :: Internet',
+    ],
+}


core/unit_tests/_testing.py

+
+    def __exit__(self, exc_type, exc_val, exc_tb):
+        import os
+        os.remove(self.name)


tox.ini

+      --cov-append \
+      --cov-config {toxinidir}/.coveragerc \
+      core/unit_tests
+    coverage report --show-missing --fail-under=100


Only files have been moved for now. Will be made into a proper package shortly. Done via: $ mkdir -p core/google/cloud/streaming $ cp google/__init__.py core/google/__init__.py $ git add core/google/__init__.py $ cp google/cloud/__init__.py core/google/cloud/__init__.py $ git add core/google/cloud/__init__.py $ git mv google/cloud/_helpers.py core/google/cloud/_helpers.py $ git mv google/cloud/_testing.py core/google/cloud/_testing.py $ git mv google/cloud/client.py core/google/cloud/client.py $ git mv google/cloud/connection.py core/google/cloud/connection.py $ git mv google/cloud/credentials.py core/google/cloud/credentials.py $ git mv google/cloud/environment_vars.py core/google/cloud/environment_vars.py $ git mv google/cloud/exceptions.py core/google/cloud/exceptions.py $ git mv google/cloud/iterator.py core/google/cloud/iterator.py $ git mv google/cloud/operation.py core/google/cloud/operation.py $ git mv google/cloud/streaming/__init__.py core/google/cloud/streaming/__init__.py $ git mv google/cloud/streaming/buffered_stream.py core/google/cloud/streaming/buffered_stream.py $ git mv google/cloud/streaming/exceptions.py core/google/cloud/streaming/exceptions.py $ git mv google/cloud/streaming/http_wrapper.py core/google/cloud/streaming/http_wrapper.py $ git mv google/cloud/streaming/stream_slice.py core/google/cloud/streaming/stream_slice.py $ git mv google/cloud/streaming/transfer.py core/google/cloud/streaming/transfer.py $ git mv google/cloud/streaming/util.py core/google/cloud/streaming/util.py

Also changing the version from 0.19.0 to 0.20.0dev. Done by adding new setup.py, MANIFEST and README to core subdirectory, adding core to the list of packages in verify_included_modules, updating the umbrella setup to depend on core and adding the local core package to the umbrella tox config.

See googleapis#2391 for larger discussion.

Done via: $ mkdir -p core/unit_tests/streaming $ cp unit_tests/__init__.py core/unit_tests/__init__.py $ git add core/unit_tests/__init__.py $ cp unit_tests/streaming/__init__.py core/unit_tests/streaming/__init__.py $ git add core/unit_tests/streaming/__init__.py $ git mv unit_tests/test__helpers.py core/unit_tests/test__helpers.py $ git mv unit_tests/test_client.py core/unit_tests/test_client.py $ git mv unit_tests/test_connection.py core/unit_tests/test_connection.py $ git mv unit_tests/test_credentials.py core/unit_tests/test_credentials.py $ git mv unit_tests/test_exceptions.py core/unit_tests/test_exceptions.py $ git mv unit_tests/test_iterator.py core/unit_tests/test_iterator.py $ git mv unit_tests/test_operation.py core/unit_tests/test_operation.py $ git mv unit_tests/streaming/test_buffered_stream.py core/unit_tests/streaming/test_buffered_stream.py $ git mv unit_tests/streaming/test_exceptions.py core/unit_tests/streaming/test_exceptions.py $ git mv unit_tests/streaming/test_http_wrapper.py core/unit_tests/streaming/test_http_wrapper.py $ git mv unit_tests/streaming/test_stream_slice.py core/unit_tests/streaming/test_stream_slice.py $ git mv unit_tests/streaming/test_transfer.py core/unit_tests/streaming/test_transfer.py $ git mv unit_tests/streaming/test_util.py core/unit_tests/streaming/test_util.py

The coverage RC file for the core subpackage intentionally leaves out google.cloud._testing. This is because the core tests don't use the entire functionality of the _testing module, but the umbrella package does. By leaving the module in the google-cloud-core package, every other package can depend on it and have the test helpers ready to access.

This was necessary because some lines were only tested transitively in the umbrella package, rather than directly by the core tests.

Also updating Travis config to incorporate core package.

dhermes · 2016-09-23T16:24:53Z

@tseaver Rebased all the lower PRs so this is ready to go.

Any remaining issues that haven't been punted on? Do you want me to have a tracking issue for the remaining work:

Do the rest of the packages
Make script for running coverage report
Make script for running tox envs (Along the lines of Only run tests for modules affected by a PR #2277)
DRY in setup.py (Figure out a way to share common pieces of setup.py across subpackages #2395)
Figure out the user agent ([QUESTION] What values should we use for our user agent? #2391)
Script using twine to upload packages on tags

Also @tseaver how do you feel about my manually making a core-0.20.0dev tag and then manually pushing google-cloud-core with twine? (I'd first need to turn off the Travis auto-upload feature, since it would push on any old tag.) This way I can move to Bigtable and then do a proper release of https://github.com/GoogleCloudPlatform/google-cloud-python-happybase before the entire package move is done.

dhermes · 2016-09-23T16:54:18Z

@tseaver We are green after rebase, all systems go. PTAL.

tseaver · 2016-09-23T19:13:58Z

@dhermes LGTM. One question: wouldn't we logically have different versions / tags for the different API libraries, separate from -core or the umbrella package?

dhermes · 2016-09-23T19:16:32Z

@tseaver Yes. The idea was laid out here: https://github.com/GoogleCloudPlatform/gcloud-common/issues/138#issuecomment-220830613

I was thinking we'd get all packages at 0.20.0 and then increment them all on their own schedule from there.

UPDATE: As for the tags, I figured we'd have a script that literally parses the tag name for a tag build and then determines which package needs to be pushed in that tag release.

dhermes added the packaging label Sep 20, 2016

dhermes assigned tseaver and theacodes Sep 20, 2016

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Sep 20, 2016

theacodes reviewed Sep 20, 2016

View reviewed changes

dhermes mentioned this pull request Sep 20, 2016

Making license headers in Python files more uniform #2370

Merged

dhermes force-pushed the make-core-subpackage branch from bdd4ab0 to d24c138 Compare September 20, 2016 23:12

dhermes force-pushed the make-core-subpackage branch from d24c138 to 796226a Compare September 20, 2016 23:53

dhermes force-pushed the make-core-subpackage branch from 796226a to 3ff2b58 Compare September 21, 2016 05:13

theacodes reviewed Sep 21, 2016

View reviewed changes

.travis.yml

- tox -e lint

- tox -e cover

- cd core && tox -e cover

- (cd core && tox -e cover)

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

theacodes reviewed Sep 21, 2016

View reviewed changes

core/MANIFEST.in

@@ -0,0 +1,3 @@

include README.rst

graft google

global-exclude *.pyc

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

dhermes force-pushed the make-core-subpackage branch from f35ce35 to 7fbc61f Compare September 21, 2016 17:41

tseaver reviewed Sep 21, 2016

View reviewed changes

dhermes force-pushed the make-core-subpackage branch 2 times, most recently from f9a8a5c to b950f8f Compare September 22, 2016 19:29

tseaver reviewed Sep 22, 2016

View reviewed changes

This was referenced Sep 22, 2016

[QUESTION] What values should we use for our user agent? #2391

Closed

Preparing PyLint for subpackage breakup. #2392

Merged

dhermes force-pushed the make-core-subpackage branch 2 times, most recently from e760490 to 02087b0 Compare September 22, 2016 23:01

dhermes mentioned this pull request Sep 22, 2016

Figure out a way to share common pieces of setup.py across subpackages #2395

Closed

dhermes force-pushed the make-core-subpackage branch from 02087b0 to bb30d08 Compare September 22, 2016 23:17

dhermes mentioned this pull request Sep 23, 2016

Moving test _helpers back into library. #2394

Merged

dhermes force-pushed the make-core-subpackage branch from bb30d08 to 055716f Compare September 23, 2016 16:09

dhermes added 7 commits September 23, 2016 09:18

Changing user-agent to use the version from google-cloud-core.

30cc67c

See googleapis#2391 for larger discussion.

Making coverage pass in core tox environment.

ed2dc92

This was necessary because some lines were only tested transitively in the umbrella package, rather than directly by the core tests.

Incorporating core package into umbrella coverage.

8643a91

Also updating Travis config to incorporate core package.

dhermes force-pushed the make-core-subpackage branch from 055716f to 8643a91 Compare September 23, 2016 16:19

dhermes merged commit fa13009 into googleapis:master Sep 23, 2016

dhermes deleted the make-core-subpackage branch September 23, 2016 19:17

This was referenced Sep 23, 2016

[TRACKING] Move all packages into subpackages #2404

Closed

Move bigtable code into a subpackage #2410

Merged

Move storage code into a subpackage #2416

Merged


		PACKAGE_ROOT = os.path.abspath(os.path.dirname(__file__))

		with open(os.path.join(PACKAGE_ROOT, 'README.rst')) as file_obj:

Moving core into designated package. #2367

Moving core into designated package. #2367

Conversation

dhermes commented Sep 20, 2016 • edited

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

theacodes commented Sep 20, 2016

dhermes commented Sep 20, 2016

dhermes commented Sep 20, 2016

theacodes commented Sep 20, 2016

dhermes commented Sep 20, 2016

dhermes commented Sep 21, 2016

dhermes commented Sep 21, 2016 • edited

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

tseaver left a comment

Choose a reason for hiding this comment

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

theacodes commented Sep 21, 2016 • edited

dhermes commented Sep 22, 2016

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

dhermes commented Sep 23, 2016

dhermes commented Sep 23, 2016

tseaver commented Sep 23, 2016

dhermes commented Sep 23, 2016 • edited

dhermes commented Sep 20, 2016 •

edited

dhermes commented Sep 21, 2016 •

edited

theacodes commented Sep 21, 2016 •

edited

dhermes commented Sep 23, 2016 •

edited