Skip to content

Conversation

@dnephin
Copy link

@dnephin dnephin commented Aug 5, 2014

This change will allow users to install the latest docker-py which adds .dockerignore support.

It is also much easier to manage dependencies with a version, instead of trying to merge in patches from another repo.

If someone needs to run the latest docker-py (as I do), pip supports git urls (http://pip.readthedocs.org/en/latest/reference/pip_install.html#git), so you can set a requirement of the master branch. This also lets you run your against your own fork if the upstream is slow to merge in PRs.

I've verified that script/build-linux, and script/build-osx still works (as well as the test suite). Let me know if there are any other tests I should run.

Copy link
Author

Choose a reason for hiding this comment

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

I noticed this was wrong. It needs to install requirements.txt first because that has pinned versions, after it can install fig itself and all deps should already be installed.

@aanand
Copy link

aanand commented Aug 5, 2014

I'm afraid we've looked at this before and it didn't work out: #264

@dnephin
Copy link
Author

dnephin commented Aug 5, 2014

Ok, I read over #264, but I don't see any blockers.

The two issues were pyinstaller version of six and docker-py missing a tag.

I've verified that pyinstaller works fine now by running both of the build scripts. Can you still reproduce the error you saw before?

docker-py is well past the versions that was required, and that was never an issue in the first place because you can pin to any git sha.

Managing docker-py this way means that fig users will always be behind in features and will require a lot more work from you to merge in updates and fixes. At the very least you could pin docker-py to your forked version if you have concerns with that project.

@dnephin
Copy link
Author

dnephin commented Aug 5, 2014

It looks like the travis failure was just a connection failure downloading pyinstaller (the python 2.7 build passed). Could I bother you to re-run that build? Thanks!

@bfirsh
Copy link

bfirsh commented Aug 5, 2014

We don't have a forked version – fig.packages.docker is exactly the same as upstream docker-py. We vendor it for annoying python packaging reasons.

Thanks for the contribution though! Maybe we should add a readme to fig.packages explaining why it exists. /cc @aanand

@dnephin
Copy link
Author

dnephin commented Aug 5, 2014

@bfirsh I read that argument but I don't understand it. Python packaging has come a long way in the last year, and I work with it daily without any real problems. Is there anything specific that prevents this? Also why would docker-py be such a special case? Why not vendor every dependency? It seems to be working well enough for other dependencies (and basically every other python project).

I understand that docker-py is not a fork, but it's also way behind master. It's currently not possible to get .dockerignore support with fig because of this. Although the short term fix for this is just to upgrade, it doesn't resolve the bigger issue of being stuck with the vendored version. It will always be behind the official release. Vendoring prevents others from using their own fork of docker-py without also maintaining a fork of fig.

Thank you for re-running the travis build.

@bfirsh
Copy link

bfirsh commented Aug 5, 2014

  1. We are very closely tied to docker-py so we're always going to pin to a specific version.

  2. We often make upstream changes to docker-py and we want to be able to make Fig releases without waiting for docker-py releases

  3. We need to use a vendored copy of six for pyinstaller which also means vendoring docker-py

@bfirsh
Copy link

bfirsh commented Aug 5, 2014

Besides – .dockerignore support hasn't even been released yet. If we wanted to include it, we'd have to vendor it!

@dnephin
Copy link
Author

dnephin commented Aug 5, 2014

@bfirsh No! That's exactly what I'm trying to say, you never need to vendor it, because python packaging is awesome! It looks like I forgot to link to the docs: http://pip.readthedocs.org/en/latest/reference/pip_install.html#git

I created this branch so that I could use .dockerignore

  1. You can do this without vendoring! Just pin a git tag, or sha, or a previous release if you really want to
  2. You can do this without vendoring! Just pin a git tag or sha
  3. I haven't been able to reproduce this issue, but if it's still an issue, you can just pin to the previous version of six! Still don't need vendoring.

Pros for vendoring:

  • None, you can do everything with a requirements.txt

Cons for vendoring:

  • you're always behind master
  • it's more work to upgrade (apply diffs instead of just changing a version number)
  • fig is less useful to developers because they can't use a custom version of docker-py directly, and have to wait on upgrades

@bfirsh
Copy link

bfirsh commented Aug 5, 2014

Sorry @dnephin, I appreciate your enthusiasm, but we have spent a lot of time struggling with Python packaging.

Referencing git tags only works with pip, and we can't guarantee that people will install using pip.

The problem is pyinstaller comes with a built-in six package that it injects into the start of PYTHONPATH, so the only way to use a more recent version of six is by vendoring it.

@dnephin
Copy link
Author

dnephin commented Aug 5, 2014

Have you considered moving away from pyinstaller? From #102 it's pretty clear pyinstaller is causing more than one issue, and there are better tools out there (https://github.com/jordansissel/fpm)

Ok, if six is a blocker, I can add that back, but I still think the docker-py dependency is a win.

@bfirsh
Copy link

bfirsh commented Aug 5, 2014

We have no plans to switch from a static binary, I'm afraid. We did that because we suffered so much pain from Python packaging.

@bfirsh
Copy link

bfirsh commented Aug 5, 2014

We still need to vendor docker-py to be able to reference our vendored version of six.

@dnephin
Copy link
Author

dnephin commented Aug 5, 2014

I must say that I am disappointed that you have given up on python packaging being a workable solution for managing dependencies in python. All I can say is that I have not had these problems, and that I guess I will have to maintain a fork with this support.

@dnephin
Copy link
Author

dnephin commented Aug 14, 2014

Re-opening this issue. There is another issue it would solve (now it's both tls and .dockerignore).

I'd also like to re-iterate that I've tested this with scripts/build-linux and it builds correctly. The executable it produces also works just fine without a vendored six. I'm not sure what issue you were hitting before, but it's not an issue anymore.

@dnephin dnephin reopened this Aug 14, 2014
@bfirsh
Copy link

bfirsh commented Aug 14, 2014

@dnephin it only occurs on specific versions of OS X, if I recall correctly. We can look into removing the vendored package, but we don't have the bandwidth to battle Python packaging at the moment.

If you want to help vendor the current master of docker-py, we'll gladly accept that!

@bfirsh
Copy link

bfirsh commented Aug 14, 2014

To be clear, the reason I'm being stubborn about this is because our primary distribution mechanism is through shipping a binary produced by PyInstaller. It's always going to have fixed dependencies.

@dnephin
Copy link
Author

dnephin commented Aug 14, 2014

I understand that. But from someone who doesn't use that mechanism, it's really frustrating to work with.

I wish I had an OSX machine to try and reproduce the error, but I'll see if I can find anything about it in the pyinstaller source or issue tracker. This can't be a unique issue with fig.

I also think that, in the long term, real packages (osxpkg, deps, rpms) are going to be quite a bit more reliable (a tool like fpm makes this easier). #102 is a good example of why I think this is the case.

I know you can use dh-virtualenv for debs so that you don't have to worry about dependencies, you can just package up everything as a python virtualenv. I'm not sure if there are equivalents for osxpkg and rpms.

@dnephin
Copy link
Author

dnephin commented Aug 14, 2014

I see pyinstaller/pyinstaller#114 is merged, but not in an official release yet. Using a git url as a requirement would still work (it is only a dev requirement).

@bfirsh
Copy link

bfirsh commented Aug 14, 2014

Okay, yes, that sounds good.

Now we're part of Docker we actually have the power to ship new versions of
docker-py too. ;)

On Thu, Aug 14, 2014 at 10:57 AM, Daniel Nephin notifications@github.com
wrote:

I see pyinstaller/pyinstaller#114
pyinstaller/pyinstaller#114 is merged, but not
in an official release yet. Using a git url as a requirement would still
work (it is only a dev requirement).


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

@dnephin
Copy link
Author

dnephin commented Aug 15, 2014

Pin to current master of pyinstaller, fix setup.py:tests_require and Dockerfile to work with git pin

@bfirsh
Copy link

bfirsh commented Aug 15, 2014

Does this mean we're still pinning versions for the build we make with PyInstaller? We definitely want to pin to specific versions for that.

@dnephin
Copy link
Author

dnephin commented Aug 15, 2014

Yup, we install pinned requirements from requirements-dev.txt and requirements.txt first, then when it goes to install fig itself, it sees that all the dependencies are already satisfied, so it doesn't install anything else.

@bfirsh
Copy link

bfirsh commented Aug 15, 2014

Hmm. I think in https://github.com/docker/fig/blob/master/script/build-osx venv/bin/pip install . should be venv/bin/pip install -r requirements.txt then. We should fix that.

@dnephin
Copy link
Author

dnephin commented Aug 15, 2014

Ah yes, you're right. I'll fix that

Dockerfile Outdated
Copy link

Choose a reason for hiding this comment

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

Could we add git to the first apt-get install so the caching works right?

Dockerfile Outdated
Copy link

Choose a reason for hiding this comment

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

These are added separately so they are cached.

Copy link
Author

Choose a reason for hiding this comment

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

Ah, I didn't realize that worked

Copy link

Choose a reason for hiding this comment

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

Yep – a few versions ago, Docker started storing the hashsum of the tarball of an ADD to detect if the contents have changed. It's pretty sweet.

@prologic
Copy link

+1

1 similar comment
@satyrius
Copy link

+1

@dnephin
Copy link
Author

dnephin commented Aug 24, 2014

I believe I've addressed all open issues here. Let me know if there is anything else.

@bfirsh
Copy link

bfirsh commented Sep 4, 2014

Great. Integration tests pass, Linux build works, OS X build works.

@bfirsh
Copy link

bfirsh commented Sep 4, 2014

LGTM

1 similar comment
@aanand
Copy link

aanand commented Sep 4, 2014

LGTM

Signed-off-by: Daniel Nephin <dnephin@gmail.com>
@dnephin dnephin force-pushed the replace_packages_with_deps branch from 03c9639 to 4d0f52b Compare September 5, 2014 02:02
@dnephin
Copy link
Author

dnephin commented Sep 5, 2014

rebased with master to resolve merge conflicts

@bfirsh
Copy link

bfirsh commented Sep 5, 2014

@dnephin D'oh. Needs another rebase.

@bfirsh
Copy link

bfirsh commented Sep 5, 2014

Tell you what I'll do it for you.

@bfirsh bfirsh closed this in 41ee65b Sep 5, 2014
@bfirsh
Copy link

bfirsh commented Sep 5, 2014

Thanks for your help with this @dnephin! Glad we got there in the end :D

@thaJeztah
Copy link
Member

Happy to see this merged as well, and a big thank you @dnephin for being persistent 😺

@dnephin dnephin deleted the replace_packages_with_deps branch September 10, 2014 02:42
yuval-k pushed a commit to yuval-k/compose that referenced this pull request Apr 10, 2015
Closes docker#375

Signed-off-by: Yuval Kohavi <yuval.kohavi@gmail.com>
xulike666 pushed a commit to xulike666/compose that referenced this pull request Jan 19, 2017
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.

6 participants