Skip to content
This repository has been archived by the owner on Sep 12, 2018. It is now read-only.

Next #395

Closed
wants to merge 14 commits into from
Closed

Next #395

wants to merge 14 commits into from

Conversation

dmp42
Copy link
Contributor

@dmp42 dmp42 commented May 29, 2014

This should come into the 0.8 branch (that will be master as soon as we switch over 0.7).

Mainly this is (almost trivial) cleanup and simplification of build / packaging / setup scripts.

Also, it fixes:

And add more (removal) tests to the test suite.

Mangled Deutz added 14 commits May 27, 2014 12:33
Docker-DCO-1.1-Signed-off-by: Mangled Deutz <[email protected]> (github: dmp42)
Docker-DCO-1.1-Signed-off-by: Mangled Deutz <[email protected]> (github: dmp42)
…zed in lib/__init__.py

Docker-DCO-1.1-Signed-off-by: Mangled Deutz <[email protected]> (github: dmp42)
Docker-DCO-1.1-Signed-off-by: Mangled Deutz <[email protected]> (github: dmp42)
Docker-DCO-1.1-Signed-off-by: Mangled Deutz <[email protected]> (github: dmp42)
Docker-DCO-1.1-Signed-off-by: Mangled Deutz <[email protected]> (github: dmp42)
Docker-DCO-1.1-Signed-off-by: Mangled Deutz <[email protected]> (github: dmp42)
Docker-DCO-1.1-Signed-off-by: Mangled Deutz <[email protected]> (github: dmp42)
Docker-DCO-1.1-Signed-off-by: Mangled Deutz <[email protected]> (github: dmp42)
Docker-DCO-1.1-Signed-off-by: Mangled Deutz <[email protected]> (github: dmp42)
Docker-DCO-1.1-Signed-off-by: Mangled Deutz <[email protected]> (github: dmp42)
Docker-DCO-1.1-Signed-off-by: Mangled Deutz <[email protected]> (github: dmp42)
Docker-DCO-1.1-Signed-off-by: Mangled Deutz <[email protected]> (github: dmp42)
Docker-DCO-1.1-Signed-off-by: Mangled Deutz <[email protected]> (github: dmp42)
@samalba
Copy link
Contributor

samalba commented May 29, 2014

About pip install docker-registry[bugsnag], how does it work? I guess it's standard, recent addition to pip?

@wking
Copy link
Contributor

wking commented May 29, 2014

On Thu, May 29, 2014 at 08:02:16AM -0700, Mangled Deutz wrote:

  • Packaging clean-up

Instead of:

requirements-style.txt
requirements-test.txt
requirements.txt

I prefer the reduced clutter of:

requirements/style.txt
requirements/test.txt
requirements/default.txt

I like the bit with extras_require. Hadn't seen that before.

  • More packaging cleanup + version and package metadata is now
    centralized in lib/init.py

You shift this to server/init.py later, so you should probably
squash that commit into this one. Personally, I'd prefer to have this
metadata in docker_registry/init.py, which should allow you to
avoid the execfile wonkiness in 7b7d021 (Fix namespace collision,
2014-05-23).

  • More cleanup

I don't understand the “(pip)” parentheticals. Is that suggesting
pip as the installer for the subsequent packages?

There's also some unneccessary churn in requirements.txt. If you want
to churn that file, I'd suggest just alphabetizing the whole thing ;).

  • Fix workflow test

This adds colorama and assorted stuff that's (mostly?) removed again
by c943b41 (Remove debug, 2014-05-29). Maybe drop both commits? Or
squash them together?

You also comment out some cookie stuff, that you should probably just
remove entirely. It's in the version control history if folks want to
resurrect it.

  • Fix namespace collision

What does the namespace breakage look like? Can't all this metadata
live in a boring docker_registry/init.py?

@dmp42
Copy link
Contributor Author

dmp42 commented May 29, 2014

@samalba @wking extras requirements are a setup tools feature: https://pythonhosted.org/setuptools/setuptools.html#declaring-extras-optional-features-with-their-own-dependencies
It got implemented in pip around 2011 I think (see over there for more: pypa/pip#7).

The idea is sexy, being able to name a "feature" and attach an array of dependencies - the downside is when installing from filesystem (it turns ugly).

@wking thanks for the thorough review

[snip] churning, confusing commits

Yes, that's a bit messy, sorry for that, and I'll squash / reformat.

About requirements/test - style, folder vs. root, yes, why not group them. Ultimately, we may even get rid of them and use a docker-python-dev package to express tooling for all docker python development if we decide we need coherence.

You shift this to server/init.py late. Personally, I'd prefer to have this
metadata in docker_registry/init.py

Well, that's not possible - that's the caveat of python namespaces: you can't have anything package specific in the init file of a namespaced folder.

which should allow you to avoid the execfile wonkiness in 7b7d021 (Fix namespace collision,
2014-05-23).

No, unfortunately.
Another shit-trap of python namespaces, but you simply can't import something that is in a namespace that way (flat) from inside a setup.py without breaking the other things installed in that namespace.

I don't understand the “(pip)” parentheticals. Is that suggesting pip as the installer for the subsequent packages?

Yes, and you are right, it's unclear. Will fix.

What does the namespace breakage look like? Can't all this metadata live in a boring docker_registry/init.py?

No, unfortunately.

See pypa/pip#3 (comment) http://stackoverflow.com/questions/17338925/cannot-install-two-packages-that-use-the-same-namespace for a starter.

@wking
Copy link
Contributor

wking commented May 29, 2014

On Thu, May 29, 2014 at 11:03:50AM -0700, Mangled Deutz wrote:

Ultimately, we may even get rid of them and use a docker-python-dev
package to express tooling for all docker python development if we
decide we need coherence.

I like explicit requirement files more than splitting the dependencies
out into a separate virtual package. You'll still need the
requirements listed somewhere, and it seems simplest to list them in
the package that's doing the imports and using the APIs ;).

You shift this to server/init.py late. Personally, I'd prefer
to have this metadata in docker_registry/init.py

Well, that's not possible - that's the caveat of python namespaces:
you can't have anything package specific in the init file of a
namespaced folder.

With PEP 420 and Python 3.3, namespace packages move into the standard
library 1, but they will still lack an init.py 2. Why do we
need a namespace package, though?

@wking
Copy link
Contributor

wking commented May 29, 2014

On Thu, May 29, 2014 at 11:44:35AM -0700, W. Trevor King wrote:

Thu, May 29, 2014 at 11:03:50AM -0700, Mangled Deutz:

You shift this to server/init.py late. Personally, I'd prefer
to have this metadata in docker_registry/init.py

Well, that's not possible - that's the caveat of python namespaces:
you can't have anything package specific in the init file of a
namespaced folder.

With PEP 420 and Python 3.3, namespace packages move into the standard
library [1], but they will still lack an init.py [2]. Why do we
need a namespace package, though?

I see that namespaces landed with a181c13 (Move away content from
init to let namespace, 2014-05-12, #353), but I can't find notes
explaining the motivation for using namespaces. We should be able to
plugin storage backends etc. even if they don't live under a single
namespace (see, for example, a8bd4a4, index: Expand load() to handle
arbitrary module names, 2014-02-17, #247).

@dmp42
Copy link
Contributor Author

dmp42 commented May 29, 2014

I like explicit requirement files more than splitting the dependencies out into a separate virtual package. You'll still need the requirements listed somewhere, and it seems simplest to list them in the package that's doing the imports and using the APIs ;).

The idea I'm talking about is to enforce common tooling across several different projects, but that's irrelevant and not going to happen (soon) for the registry itself.

With PEP 420 and Python 3.3, namespace packages move into the standard library [1], but they will still lack an init.py [2].

I know PEP 420 but unfortunately, moving to python3 and dropping py2 support is science fiction here (gevent is not compatible yet, and flask broke it), so we have to stick with the old crappy way.

Why do we need a namespace package, though?

The PR you link to actually asked for comment :-)

Either way, there are a number of advantages in using namespaces, especially not multiplying different global names between different packages (-core, -registry), or ability to list all available drivers by enumerating docker_registry.drivers, etc.

Sure, there are other ways to do "plugins" - that one has upsides and downsides (admittedly I'm disappointed about how broken python namespaces / packaging are, but it's still a powerful and clean formalism).

Best.

@dmp42 dmp42 added this to the 0.8 milestone May 29, 2014
@dmp42
Copy link
Contributor Author

dmp42 commented May 29, 2014

  • Fix workflow test: This adds colorama and assorted stuff that's (mostly?) removed again by c943b41 (Remove debug, 2014-05-29). Maybe drop both commits? Or squash them together?

This more essentially makes workflow usable again - it is entirely broken on master, and has been for quite some time.

I'll clean up the commits though, to avoid adding / removing debug helpers.

@samalba
Copy link
Contributor

samalba commented May 29, 2014

lgtm

@wking
Copy link
Contributor

wking commented May 29, 2014

On Thu, May 29, 2014 at 02:01:15PM -0700, Mangled Deutz wrote:

Why do we need a namespace package, though?

The PR you link to actually asked for comment :-)

It was a big PR, and I was too busy to look it over ;). Apologies for
the late review.

Either way, there are a number of advantages in using namespaces,
especially not multiplying different global names between different
packages (-core, -registry),

Meh :p. This doesn't seem to be worth any implementation
hoop-jumping.

or ability to list all available drivers by enumerating
docker_registry.drivers, etc.

Is this actually useful? Users will have to know which drivers are
available beforehand (so they can configure the one they want).
Having a Python oneliner to list installed driver packages (in case
you forgot after installing them?) doesn't sound useful enough to me
for the associated namespace-module hoop jumping. I'm happy to be
wrong here, I just don't see the utility yet.

I'm still not sure why this blows up and requires the execfile hack
from 7b7d021 (Fix namespace collision, 2014-05-23). Is it a quirk of
the setuptools implementation? Are you seeing:

'error: package directory '…' does not exist'

as in 1? How can I reproduce the error you were working around
locally?

@dmp42
Copy link
Contributor Author

dmp42 commented May 29, 2014

It was a big PR, and I was too busy to look it over ;). Apologies for the late review.

Peace :-)

[@wking doesn't like namespaces, at all, he thinks they are all ugly, and smelly, and they are bad and... :-)]

Well I do like namespaces, and do wish they were less "hoopy" :-)

Now:

  • we are talking (lengthily) about a one-line inconvenience in a setup file
  • I don't see a such a minor "problem" (<- I paid the price researching the origin of it) as a good reason enough to change a design and discard quite a lot of work

I completely agree that you have reasons not to like namespaces (I too was bitten when I was young - it was by a PEAR package though :-)) but I can assure you namespaces can be good if you tame them:

  • easy to scaffold, normalized drivers all sharing the exact same structure and folders is good
  • consistent naming is good (yes, I'm a bit of a naming freak :-))
  • ability to run tests for all installed drivers from the registry package itself is good
  • ability to programmatically report what drivers are installed is good (unless you really think you can trust users to report accurately what they did :-))

Anyhow, we are entirely OT here (*), and I'm on a GMT timezone :-)

((*) I'd be happy to have a beer with you someday, though)

@wking
Copy link
Contributor

wking commented May 29, 2014

On Thu, May 29, 2014 at 03:05:51PM -0700, Mangled Deutz wrote:

Anyhow, we are entirely OT here (*), and I'm on a GMT timezone :-)

Heh, works for me.

@wking
Copy link
Contributor

wking commented May 29, 2014

On Thu, May 29, 2014 at 03:05:51PM -0700, Mangled Deutz wrote:

[@wking doesn't like namespaces, at all, he thinks they are all
ugly, and smelly, and they are bad and... :-)]

Well I do like namespaces, and do wish they were less "hoopy" :-)

And I like PEP 420, and was able to get a docker_registry.meta import
working without hoops using Python 3.3 ;). I just don't like
setuptools or other outside-the-stdlib packaging. Packaging is too
important to trust to something as unstable and poorly spec-ed as an
external package :p.

@dmp42
Copy link
Contributor Author

dmp42 commented May 30, 2014

Hear hear :-)

@dmp42 dmp42 closed this May 30, 2014
@dmp42 dmp42 deleted the next branch May 30, 2014 15:29
@dmp42 dmp42 mentioned this pull request May 30, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants