Upgrade websocket-client to 1.2.1

[PureTryOut]

Description

This is the core equivalent of MycroftAI/mycroft-messagebus-client#21
There was an incompability with the latest websocket-client and the
messagebus which needed fixing for Linux distro compatibility. Since
messagebus-client was going to depend on websocket-client 0.58.0, let's
do the same in core

How to test

Run this combined with MycroftAI/mycroft-messagebus-client#21 and an upgraded websocket-client, see if everything still works as expected.

Contributor license agreement signed?

CLA [x]

[krisgesling]

Since we're now importing the message-bus-client, should we be removing this from Mycroft-core's requirements instead of trying to keep it in sync?

From a quick glance it seems like we're still using it in two locations

• send() from send_func.py appears to be in the message bus client so should be imported
• the GUI websocket... that will take some further investigation...

In which case this seems like a reasonable short term solution. Can you increment the bus client version requirement also and I'll update the package on PyPI?

[PureTryOut]

should we be removing this from Mycroft-core's requirements instead of trying to keep it in sync?

Seems like a good idea.

Can you increment the bus client version requirement also and I'll update the package on PyPI?

Will do.

[devops-mycroft]

Voight Kampff Integration Test Succeeded (Results)

[forslund]

Also see #2878 (Mainly for the discussion)

[devops-mycroft]

Voight Kampff Integration Test Failed (Results).
Mycroft logs are also available: skills.log, audio.log, voice.log, bus.log, enclosure.log

[forslund]

@PureTryOut can you change test/integrationtests/voight_kampff/features/environment.py#L47 to match the new format

def on_message(self, _, message):

to match the changes. That should fix the VK issues

[devops-mycroft]

Voight Kampff Integration Test Failed (Results).
Mycroft logs are also available: skills.log, audio.log, voice.log, bus.log, enclosure.log

[PureTryOut]

@forslund doesn't seem to fix it sadly 😢

[forslund]

Darn, will re-check tonight*

• Might not actually be tonight

[forslund]

Ah, it's probably that line 51 the _ needs to be passed on to the super() as well...

super().on_message(_, message)

[forslund]

Now the tests completed atleast :)

[devops-mycroft]

Voight Kampff Integration Test Failed (Results).
Mycroft logs are also available: skills.log, audio.log, voice.log, bus.log, enclosure.log

[forslund]

Wikipedia skill seems to have issues with science at the moment, seems to be some change on the wikipedia site making it not catch science properly at the moment.

The changes looks very correct.

[forslund]

I was made aware by a user that the mark-2 enclosure's "gui" bus needs to be updated as well.

I think it's only https://github.com/MycroftAI/mycroft-core/blob/dev/mycroft/client/enclosure/base.py#L496 and https://github.com/MycroftAI/mycroft-core/blob/dev/mycroft/client/enclosure/base.py#L528

As a next step we should likely rewrite that. class so it's a derived class from the mycroft-message-bus (will likely override eveything but would be better organization-wise)

[PureTryOut]

@forslund updated! The rewriting can be done in some other PR 😉

[codecov-commenter]

Codecov Report

Merging #2879 (b11371a) into dev (9e9f2f7) will increase coverage by 0.22%.
The diff coverage is 91.30%.

❗ Current head b11371a differs from pull request most recent head df5c5b3. Consider uploading reports for the commit df5c5b3 to get more accurate results

@@            Coverage Diff             @@
##              dev    #2879      +/-   ##
==========================================
+ Coverage   52.80%   53.02%   +0.22%     
==========================================
  Files         123      123              
  Lines       11106    11156      +50     
==========================================
+ Hits         5865     5916      +51     
+ Misses       5241     5240       -1     

Impacted Files	Coverage Δ
mycroft/client/enclosure/base.py	0.00% <0.00%> (ø)
mycroft/skills/intent_services/adapt_service.py	80.28% <50.00%> (-0.44%)	⬇️
mycroft/skills/common_query_skill.py	93.61% <93.47%> (+7.17%)	⬆️
mycroft/skills/intent_service.py	68.85% <100.00%> (+1.13%)	⬆️
mycroft/skills/intent_service_interface.py	35.38% <100.00%> (+1.35%)	⬆️
mycroft/skills/mycroft_skill/mycroft_skill.py	74.73% <100.00%> (+0.04%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9e9f2f7...df5c5b3. Read the comment docs.

[devops-mycroft]

Voight Kampff Integration Test Failed (Results).
Mycroft logs are also available: skills.log, audio.log, voice.log, bus.log, enclosure.log

[devops-mycroft]

Voight Kampff Integration Test Failed (Results).
Mycroft logs are also available: skills.log, audio.log, voice.log, bus.log, enclosure.log

[penrods]

Since you are talking about websocket-client, I'll bring up a couple things. The license on it is...messy. I'm working with them to get it officially cleaned up and transitioned to Apache 2.0, but the 0.5.x versions have a questionable BSD license.

The 1.x series is also fully Python 3, for what it is worth. Once the 1.0.2 (or maybe 1.1.0) version comes out, I'd recommend switching to it just to eliminate any concerns about licenses.

[krisgesling]

Hey Steve, thanks for the heads up - and for your work in helping another project navigate the painful world of re-licensing!

By questionable do you mean - it is questionable whether the code is legitimately licensed BSD or do you think there is a problem with us using code licensed under a BSD 3-clause license?

It looks like the license got reverted to LGPLv2.1 for v0.58 which is what we'd be including here. This seems more problematic.

Definitely seems like an Apache 2.0 licensed v1.x will be worth upgrading too once we know it's stable...

I think we need an official call on this, so will have a chat internally and report back soon.

[forslund]

Looks good now! Thanks for doing this @PureTryOut

[krisgesling]

Hi @PureTryOut thanks for all the work on this - it's a good change however we need to hold it off for the moment.

I've marked this PR as blocked as it would be introducing an LGPLv2.1 licensed library into mycroft-core which is not something we are prepared to do.

The current version of the websocket-client has been labeled with a BSD 3-clause license and we're only now aware that there might be some questions about that. Given the project is actively working toward remediation by switching to an Apache 2.0 license with full informed and express consent from all contributors - I think it's best that we hold all version updates for this package for the moment. They are making great progress at reaching all contributors to their project and it seems like they will be able to update the license sometime in June.

As this version update has already been applied to the mycroft-messagebus-client v0.9.2 I'll be adding a warning to that repo and the specific release notes so that any projects can make their own decision on whether they use it or not. This also means that mycroft-messagebus-client v0.9.2 cannot be used in other Mycroft projects. It will need to be updated with an appropriately licensed package.

I want to highlight that from reading their Github issue threads it really does seem like the maintainers are well-intentioned and want the license to be as open as possible - they just weren't across all of the detail of the license and what's required to change the license on a project that has had so many contributions from 3rd parties. This is a complex area of law for lawyers and I think it sucks that developers are expected to be quasi copyright lawyers but it is something we have to be aware of and careful about. So kudos to them for building a great project and for going through this period of pain so that the broader community can continue to benefit from it.

[forslund]

Just to point out, the current situation is that the 0.54 BSD license is probably not actually a BSD license since it was changed in a bad way. So the current 0.54 is likely LGPL if I'm reading things correctly...

I think it may be better to move off of the 0.54 release since that is using a contested BSD license and we accepted the license before when it was LGPL.

But hopefully at the end of the month the license issue will have been cleared up and they'd moved to the Apache License in a legal manner.

[PureTryOut]

I'm fine with any approach mentioned by the way. I'm just keeping this patch downstream for now till either this or something else is merged to fix it.

[krisgesling]

It's a tricky situation and I don't think there is a right answer. Even trying to switch the project back to LGPL doesn't really work as any code that was contributed during the BSD phase was done so under a BSD license, not LGPL.

We have decided to leave the current version of the package as it is for the moment. From what I've seen they have so far had no push back on the switch to Apache, and the list of remaining contributors is getting smaller and smaller. So hopefully all goes smoothly with the rest and we can jump forward to v1.x shortly.

[JarbasAl]

I would not trust the BSD license to be valid, it would apply to code contributed after the switch, but it is still GPL "contaminated" which would automatically convert/dual-license the whole work as GPL regardless, at best different parts of the package use different licenses

the correct thing to do is to wait for the migration to apache-2.0 imho, and if that doesnt happen this lib should be dropped, there are plenty websocket libs out there.

I will also wait for the apache license change in all my packages since it seems to be happening quite fast, but this makes me very uncomfortable, did any of the code i wrote get contaminated by GPL? do i need to review the full commit history of every package i use to ensure any license change was valid?

I want to code not be a lawyer...

/end rant

[penrods]

I would not trust the BSD license to be valid, it would apply to code contributed after the switch, but it is still GPL "contaminated" which would automatically convert/dual-license the whole work as GPL regardless, at best different parts of the package use different licenses

the correct thing to do is to wait for the migration to apache-2.0 imho, and if that doesnt happen this lib should be dropped, there are plenty websocket libs out there.

I will also wait for the apache license change in all my packages since it seems to be happening quite fast, but this makes me very uncomfortable, did any of the code i wrote get contaminated by GPL? do i need to review the full commit history of every package i use to ensure any license change was valid?

I want to code not be a lawyer...

/end rant

You shouldn't waste any time on reviewing code for GPL contamination, Jarbas. Remember, the ultimate point of a license is to allow the individuals who actually own the various lines of code to provide access to others. Given that the participants in the websocket-client project are cheerfully signing off on the Apache change, you have no reason to worry about someone with actual legal standing (one of the copyright holders) coming after you. They just want their code to be available to all and the LGPL/BSD snafu was simply a misunderstanding. There is no way they are coming after you in court for misusing their code.

[krisgesling]

Just so people know we are still watching this. They are down to 30 (out of >200) who haven't yet responded to the re-licensing thread.

[krisgesling]

Hey all - great news...

websocket-client v1.2.0 was just released and is fully Apache 2.0 licensed 🎉

[ChanceNCounter]

Does that mean this bump needs a bump?

[forslund]

@PureTryOut the mycroft-messagebus-client 0.9.3 now uses websocket-client 1.2.1 with the updated License could you update the requirements to the matching versions in this PR?

[PureTryOut]

Done!

[krisgesling]

Hey just to update on status:

• mycroft-core v21.2.1 got released today
• Chris is going to update our VK test runners to use this new version
• that should allow the new Timer Skill to get updated and clear the final blocker

So I'm expecting we can merge these in early next week.

[JarbasAl]

i got this error in mycroft.client.enclosure

 File "/home/user/my_code/mycroft_workspace/.venv/lib/python3.8/site-packages/tornado/websocket.py", line 570, in on_connection_close
    self.on_close()
TypeError: on_close() missing 1 required positional argument: '_'

bumping tornado to 6.1 made it go away

[krisgesling]

Thanks Jarbas, I've bumped the Tornado version and the bus client again.

I've also removed websocket-client as a mycroft-core dependency since this is no longer used directly and means we don't need to keep the versions synced with the bus-client package.

Everything seems to be working for me locally - so I think this is ready to roll.

[forslund]

Core has a direct dependency on websocket-client in the gui bus.

Edit: Just me being confused. Disregard...
Edit2: It has a direct dependency, in the GUI debug client in the text client. but perhaps that debug client can be decommissioned completely now.

[krisgesling]

Core has a direct dependency on websocket-client in the gui bus.

Edit: Just me being confused. Disregard...
Edit2: It has a direct dependency, in the GUI debug client in the text client. but perhaps that debug client can be decommissioned completely now.

Good spotting - I've re-added it for now. Switching that over is beyond the scope of this PR.

[PureTryOut]

@krisgesling any reason this hasn't been merged yet more than a month after you said it was ready to roll?

[krisgesling]

Hey, thanks for the nudge. There was a failing VK test and I hadn't cycled back to the PR.

Merging now!