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

Remove built dependencies #985

Closed
wants to merge 1 commit into from

Conversation

kaedroho
Copy link
Contributor

This commit removes libsass and Pillow from the setup.py dependency list. This greatly improves install performance and also means that the basic Wagtail installation is pure-python (so no build tools need to be on the end users host machine).

None of these dependencies are directly called from within Wagtail so the start project command continues to work correctly.

This commit removes libsass and Pillow from the setup.py dependency list. This greatly improves install performance and also means that the basic Wagtail installation is pure-python (so no build tools need to be on the end users host machine).

None of these dependencies are directly called from within Wagtail so the start project command continues to work correctly.
@kaedroho kaedroho added this to the 0.9 milestone Feb 11, 2015
@zerolab
Copy link
Contributor

zerolab commented Feb 11, 2015

Hi @kaedroho,

Can you add a paragraph in the documentation for the use case outlined by @gasman in #958 (comment). That is if Wagtail is added to an existing Django project rather than starting a new project using the template.

It also looks like #958 has an unanswered question from @tomdyson. Although it may be the case you already discussed this offline.

@gasman
Copy link
Collaborator

gasman commented Feb 11, 2015

+1 for removing them as dependencies of the wagtail package itself, but we need to do a proper job on the documentation - removing all reference to them is not sufficient, because not everyone's using Vagrant, and for those people the dependencies are just as necessary as they were before.

If anything, our documentation now needs to be more explicit about the dependencies, because an error during package installation made it pretty obvious that some prerequisites were missing, whereas a failure during site setup is likely to lead people to assume that Wagtail is broken. I think this is going to require a fairly major rethink of the docs, to convey all the relevant information: "this is how far installing the wagtail package is going to get you; these are the dependencies your site is going to need; here are some options to save you the hassle of installing them" etc.

@SalahAdDin
Copy link
Contributor

But, if now we haven't libsass, how you put all scss styles in wagtail admin?

@gasman
Copy link
Collaborator

gasman commented Feb 17, 2015

@SalahAdDin We aren't dropping libsass. This is just about reorganising the way we specify dependencies in the code, so that people using virtual machines (such as Vagrant) only have to install libsass within the VM, not on the host machine.

@gasman
Copy link
Collaborator

gasman commented Mar 20, 2015

Sorry, but I think we need to revert this. In the last week I've had two Torchbox sites fail to install cleanly with vagrant up because their requirements.txt files were missing these libraries - and if that's a problem on our own sites, it's going to be ten times worse when we have to troubleshoot other people's.

Having to install native dependencies on the host machine is annoying, but people have dealt with it up to now, and we have 8 releases worth of experience in dealing with those problems. It feels like we've rushed into this change without fully thinking through the implications, and the result is that we're adding another layer of "stuff that can go wrong". We're going to have our hands full dealing with support queries from the rest of the changes in 0.9 - this is an extra problem that we could do without. (And hopefully the next release will then have a proper solution for libsass - #35 (comment))

@gasman gasman reopened this Mar 20, 2015
@davecranwell
Copy link
Contributor

Sounds sensible.

gasman added a commit to gasman/wagtail that referenced this pull request Mar 24, 2015
kaedroho added a commit that referenced this pull request Mar 24, 2015
@gasman
Copy link
Collaborator

gasman commented Mar 27, 2015

re-closing, as this is now reverted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants