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

Dependencies Updates #1536

Merged
merged 19 commits into from Feb 21, 2017

Conversation

2 participants
@kevans91
Contributor

kevans91 commented Oct 7, 2016

What does this PR do and why is it necessary?

Updates select dependencies to more recent releases. It's not strictly necessary, but:

  1. The changes to make this work were actually fairly minimal/trivial
  2. Plays a little more nicely with package managers, which might now have newer versions of a lot of these dependencies.

Number two was was the motivation for this PR -- most of the dependencies that DO currently exist in the FreeBSD ports tree are already at a newer

How was it tested? How can it be tested by the reviewer?

  1. Applied changes one-by-one

  2. Figured out which functionality each one was used for, played around with that

  3. Ran nosetests, excerpt:

    ghost# venv/bin/nosetests

    ........................................................................................................................................... ........................................................................................................................................... ..................................................................................

    Ran 360 tests in 30.927s

    OK

Admittedly, I have not yet been able to really test the pyserial update. I've started using OctoPrint primarily on FreeBSD, and pyserial does not currently support non-standard baud rates on FreeBSD. I intend to correct this latter point shortly, because (from looking at pyserial) it should be even more straightforward on *BSD than it is on Linux. This testing will happen this weekend for sure.

Any background context you want to provide?

See above -- package managers, etc. etc.

What are the relevant tickets if any?

None

Screenshots (if appropriate)

None

Further notes

I suspect this may not be an entirely popular change, but I do think it's necessary and the devel branch is a good candidate for test monkey'ing. I left dependencies alone if they were still relatively close, changelog-wise. Also, I left each of the individual commits intact for this one to make it easier to back out individual version bumps. The hope was that if each one can be backed out individually, there's still a good chance that some of them will get through. =)

@kevans91

This comment has been minimized.

Show comment
Hide comment
@kevans91

kevans91 Oct 7, 2016

Contributor

I should probably note that while I didn't get a chance to thoroughly test py-serial, it does at least still properly enumerate my serial ports when I have my printer plugged in and does successfully communicate with the printer, just not at the proper baud rate (by choice).

Contributor

kevans91 commented Oct 7, 2016

I should probably note that while I didn't get a chance to thoroughly test py-serial, it does at least still properly enumerate my serial ports when I have my printer plugged in and does successfully communicate with the printer, just not at the proper baud rate (by choice).

@foosel

This comment has been minimized.

Show comment
Hide comment
@foosel

foosel Oct 7, 2016

Owner

As a word of warning, reviewing and merging this will take a while. I currently am on the final strip for a first release candidate of 1.3.0 and hence on a code freeze more or less - this definitely looks like too risky for a last minute devel change :)

Owner

foosel commented Oct 7, 2016

As a word of warning, reviewing and merging this will take a while. I currently am on the final strip for a first release candidate of 1.3.0 and hence on a code freeze more or less - this definitely looks like too risky for a last minute devel change :)

@kevans91

This comment has been minimized.

Show comment
Hide comment
@kevans91

kevans91 Oct 7, 2016

Contributor

Oh yeah, no worries. =) I pretty much expected it'd take a while, but having proposed this I can work on an OctoPrint port for FreeBSD (to make my life a little easier). This is at least a step towards a bring-up on the latest versions of the dependencies, even if it gets restructured a little bit from what I have here.

Contributor

kevans91 commented Oct 7, 2016

Oh yeah, no worries. =) I pretty much expected it'd take a while, but having proposed this I can work on an OctoPrint port for FreeBSD (to make my life a little easier). This is at least a step towards a bring-up on the latest versions of the dependencies, even if it gets restructured a little bit from what I have here.

@foosel foosel added this to the 1.4.0 milestone Nov 22, 2016

@kevans91

This comment has been minimized.

Show comment
Hide comment
@kevans91

kevans91 Nov 27, 2016

Contributor

In light of 0f53c1a, it seems to make sense to me to go ahead and sort out problems with large streamed uploads/downloads as part of this PR/dependency update. Do you happen to know off the top of your head what the problem there was?

I ask because it seems to me that server/util/tornado.py's UploadStorageFallbackHandler meets all the criteria for properly handling large request data, and I've not yet had any issues. On the other hand, that could just mean I'm not trying hard enough. =)

Contributor

kevans91 commented Nov 27, 2016

In light of 0f53c1a, it seems to make sense to me to go ahead and sort out problems with large streamed uploads/downloads as part of this PR/dependency update. Do you happen to know off the top of your head what the problem there was?

I ask because it seems to me that server/util/tornado.py's UploadStorageFallbackHandler meets all the criteria for properly handling large request data, and I've not yet had any issues. On the other hand, that could just mean I'm not trying hard enough. =)

@foosel

This comment has been minimized.

Show comment
Hide comment
@foosel

foosel Nov 28, 2016

Owner

Hm... I thought it was the large streamed uploads/downloads that were the problem, but looking closer at it, it might have actually rather been the request body size limitations. Basically route-based limiting, only possible in Tornado 4.0 by basically reimplementing a bunch of connection classes in order to pass through and check against the route specific maximum body sizes.

The reason behind that was that I wanted to severly limit the possible request body sizes as early as possible, apart from the dedicated upload endpoints, in order to prevent any case of attacks involving huge requests sent against the pi and effectively nuking its disk space and/or memory.

The stuff I'm referring to was pretty much all added in b4af85f, basically the whole server.util.tornado package back at that point, see here. Additionally there's now also the ioloop scheduling fix, but that could probably just built back and that would be it, since current Tornado versions should already contain that.

Owner

foosel commented Nov 28, 2016

Hm... I thought it was the large streamed uploads/downloads that were the problem, but looking closer at it, it might have actually rather been the request body size limitations. Basically route-based limiting, only possible in Tornado 4.0 by basically reimplementing a bunch of connection classes in order to pass through and check against the route specific maximum body sizes.

The reason behind that was that I wanted to severly limit the possible request body sizes as early as possible, apart from the dedicated upload endpoints, in order to prevent any case of attacks involving huge requests sent against the pi and effectively nuking its disk space and/or memory.

The stuff I'm referring to was pretty much all added in b4af85f, basically the whole server.util.tornado package back at that point, see here. Additionally there's now also the ioloop scheduling fix, but that could probably just built back and that would be it, since current Tornado versions should already contain that.

@kevans91

This comment has been minimized.

Show comment
Hide comment
@kevans91

kevans91 Dec 28, 2016

Contributor

So basically, it should be safe to revert (at least partially) 57de36a -- correct? Do we completely revert and still set time_func in the same fashion, or is that a thing we no longer care about so we just remove the fix_ioloop_scheduling and its associated call?

Contributor

kevans91 commented Dec 28, 2016

So basically, it should be safe to revert (at least partially) 57de36a -- correct? Do we completely revert and still set time_func in the same fashion, or is that a thing we no longer care about so we just remove the fix_ioloop_scheduling and its associated call?

@foosel

This comment has been minimized.

Show comment
Hide comment
@foosel

foosel Jan 9, 2017

Owner

I think we no longer care about that. The time_func stuff was an attempt at a fix before the monkey patch that didn't pan out. Just instantiating IOLoop should actually work with newer tornado versions including that patch.

(Also, sorry for the late reply, was in full vacation mode)

Owner

foosel commented Jan 9, 2017

I think we no longer care about that. The time_func stuff was an attempt at a fix before the monkey patch that didn't pan out. Just instantiating IOLoop should actually work with newer tornado versions including that patch.

(Also, sorry for the late reply, was in full vacation mode)

@kevans91

This comment has been minimized.

Show comment
Hide comment
@kevans91

kevans91 Jan 9, 2017

Contributor

Hi,

I've incorporated the partial revert of that commit, then. No worries for late reply, I am patient. =)

Contributor

kevans91 commented Jan 9, 2017

Hi,

I've incorporated the partial revert of that commit, then. No worries for late reply, I am patient. =)

@kevans91

This comment has been minimized.

Show comment
Hide comment
@kevans91

kevans91 Feb 15, 2017

Contributor

Hi,

Any updates on reviewing this? It's been a full month since the previous communication, and keeping this up-to-date is starting to look less and less attractive. =(

Contributor

kevans91 commented Feb 15, 2017

Hi,

Any updates on reviewing this? It's been a full month since the previous communication, and keeping this up-to-date is starting to look less and less attractive. =(

@foosel

This comment has been minimized.

Show comment
Hide comment
@foosel

foosel Feb 15, 2017

Owner

Sorry, I know... I was out sick the past two weeks, am still catching up on everything that happened since then and before that had just finally finished working through most of my backlog from my overdue vacation when I got knocked down.

I understand that that is frustrating, trust me, it is for me as well. I have this high on my to-do list, I just keep getting thrown back from actually tackling it by bugs and other such things.

Owner

foosel commented Feb 15, 2017

Sorry, I know... I was out sick the past two weeks, am still catching up on everything that happened since then and before that had just finally finished working through most of my backlog from my overdue vacation when I got knocked down.

I understand that that is frustrating, trust me, it is for me as well. I have this high on my to-do list, I just keep getting thrown back from actually tackling it by bugs and other such things.

@kevans91

This comment has been minimized.

Show comment
Hide comment
@kevans91

kevans91 Feb 15, 2017

Contributor

Alrighty, good to hear. =) It's not just OctoPrint, to be honest, I've tried contributing to a couple of different open source projects now and my patches aren't even making it to a review process. They get to a "Hey, yeah, we'll review that at some point" and then it just...doesn't happen, or takes a reallllly long time. =( A little more overall frustration than specifically here.

Contributor

kevans91 commented Feb 15, 2017

Alrighty, good to hear. =) It's not just OctoPrint, to be honest, I've tried contributing to a couple of different open source projects now and my patches aren't even making it to a review process. They get to a "Hey, yeah, we'll review that at some point" and then it just...doesn't happen, or takes a reallllly long time. =( A little more overall frustration than specifically here.

@foosel

This comment has been minimized.

Show comment
Hide comment
@foosel

foosel Feb 15, 2017

Owner

I really understand, I've been in the same position. I really want to get this merged as fast as possible now that 1.3.0 is finally out and the road on devel clear for 1.4.0. In fact just today we briefly talked about the need for dependency updates on IRC.

I still have a bunch of bugs to investigate and fix from the past two weeks, but after that I'll do my best to finally give this a thorough review and test. I'm expecting there to be issues still (I'm especially still worried about the tornado stuff and flask also had some changes that might cause issues for plugins due to eg the changed extension namespace conventions), but this update is long overdue, so that will be something I'll just have to soldier through ;)

One thing btw which we definitely cannot update past 2.8.x for now is Jinja because there was a change in there that causes some specific template constructs to stop working, and while we can fix that in OctoPrint, we can't in plugins, and we need to stay backwards compatible. Just if you are wondering why I pinned that in January all of a sudden.

Edit forgot half a sentence

Owner

foosel commented Feb 15, 2017

I really understand, I've been in the same position. I really want to get this merged as fast as possible now that 1.3.0 is finally out and the road on devel clear for 1.4.0. In fact just today we briefly talked about the need for dependency updates on IRC.

I still have a bunch of bugs to investigate and fix from the past two weeks, but after that I'll do my best to finally give this a thorough review and test. I'm expecting there to be issues still (I'm especially still worried about the tornado stuff and flask also had some changes that might cause issues for plugins due to eg the changed extension namespace conventions), but this update is long overdue, so that will be something I'll just have to soldier through ;)

One thing btw which we definitely cannot update past 2.8.x for now is Jinja because there was a change in there that causes some specific template constructs to stop working, and while we can fix that in OctoPrint, we can't in plugins, and we need to stay backwards compatible. Just if you are wondering why I pinned that in January all of a sudden.

Edit forgot half a sentence

@foosel

This comment has been minimized.

Show comment
Hide comment
@foosel

foosel Feb 21, 2017

Owner

Ok... First of all, I went through your changes and everything looks fine 👍

I was worried about the backwards incompatibilities introduced by the breaking changes in Flask (flask.ext.foo => flask_foo) and Flask-Login (is_anonymous() => is_anonymous). We can't know if plugins out there are not using anything like that. So I've added two compatibility layers that will be removed again in 1.5.0 but which should now at least log a deprecation warning but still keep things working as is, instead of the actual breaking behaviour without them. At least that's the hope.

I also got warnings by Flask because request.json is now deprecated in favor of request.get_json() so I adjusted all uses of that.

There were also a bunch of flask.ext left overs thanks to a merged change on the maintenance branch, I've adjusted those too. And finally I mirrored the implementations of the customized tornado http server methods to be closer to the stock versions again, instructed flask to auto reload templates by default (we have a cache, we depend on templates being freshly read from disk if we instruct flask/jinja to do so) and finally did another round of updates of the dependencies that saw new releases since your initial walk through.

I tested a couple of things, including printing something, and so far it looks like the updates didn't break anything obvious. Of course, that doesn't mean there aren't still some not so obvious issues in there, but the only way to find them is merging to devel and having some more people play around with this stuff I guess.

I haven't yet merged though but pushed to pr/kevans91/dep-up for now. The reason being that it would be great if you could now also take a look if that looks ok from your side. If so I'll merge to devel.

edit once I've fixed the unit tests that is that are now failing thanks to one of the compatibility layers 😞 Fixed 😄

Owner

foosel commented Feb 21, 2017

Ok... First of all, I went through your changes and everything looks fine 👍

I was worried about the backwards incompatibilities introduced by the breaking changes in Flask (flask.ext.foo => flask_foo) and Flask-Login (is_anonymous() => is_anonymous). We can't know if plugins out there are not using anything like that. So I've added two compatibility layers that will be removed again in 1.5.0 but which should now at least log a deprecation warning but still keep things working as is, instead of the actual breaking behaviour without them. At least that's the hope.

I also got warnings by Flask because request.json is now deprecated in favor of request.get_json() so I adjusted all uses of that.

There were also a bunch of flask.ext left overs thanks to a merged change on the maintenance branch, I've adjusted those too. And finally I mirrored the implementations of the customized tornado http server methods to be closer to the stock versions again, instructed flask to auto reload templates by default (we have a cache, we depend on templates being freshly read from disk if we instruct flask/jinja to do so) and finally did another round of updates of the dependencies that saw new releases since your initial walk through.

I tested a couple of things, including printing something, and so far it looks like the updates didn't break anything obvious. Of course, that doesn't mean there aren't still some not so obvious issues in there, but the only way to find them is merging to devel and having some more people play around with this stuff I guess.

I haven't yet merged though but pushed to pr/kevans91/dep-up for now. The reason being that it would be great if you could now also take a look if that looks ok from your side. If so I'll merge to devel.

edit once I've fixed the unit tests that is that are now failing thanks to one of the compatibility layers 😞 Fixed 😄

@kevans91

This comment has been minimized.

Show comment
Hide comment
@kevans91

kevans91 Feb 21, 2017

Contributor

Hi,

Whoops! My apologies for not catching the request.json bits.

Your follow-up changesets look good to me, except cf20b3a seems to introduce a soon-to-be-deprecated invocation of is_anonymous(), and this all seems to operate just as well on my RPi/FreeBSD (without attempting a test print, of course).

Thanks!

Contributor

kevans91 commented Feb 21, 2017

Hi,

Whoops! My apologies for not catching the request.json bits.

Your follow-up changesets look good to me, except cf20b3a seems to introduce a soon-to-be-deprecated invocation of is_anonymous(), and this all seems to operate just as well on my RPi/FreeBSD (without attempting a test print, of course).

Thanks!

@foosel

This comment has been minimized.

Show comment
Hide comment
@foosel

foosel Feb 21, 2017

Owner

Hah, good that you spotted that in cf20b3a - that actually was a left over from the tests I did for the compatibility layers and wasn't supposed to be committed like that :)

Owner

foosel commented Feb 21, 2017

Hah, good that you spotted that in cf20b3a - that actually was a left over from the tests I did for the compatibility layers and wasn't supposed to be committed like that :)

@foosel foosel merged commit 639619f into foosel:devel Feb 21, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@foosel

This comment has been minimized.

Show comment
Hide comment
@foosel

foosel Feb 21, 2017

Owner

And merged 🎉🎈😄

Owner

foosel commented Feb 21, 2017

And merged 🎉🎈😄

@kevans91

This comment has been minimized.

Show comment
Hide comment
@kevans91

kevans91 Feb 21, 2017

Contributor

Whoo! Thanks! =)

Contributor

kevans91 commented Feb 21, 2017

Whoo! Thanks! =)

@kevans91 kevans91 deleted the kevans91:dep-up branch Feb 21, 2017

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