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

Allow system-wide and user installations rather than just virtual environments #2802

Closed
wants to merge 2 commits into from
Closed

Allow system-wide and user installations rather than just virtual environments #2802

wants to merge 2 commits into from

Conversation

PureTryOut
Copy link
Contributor

@PureTryOut PureTryOut commented Jan 15, 2021

This is a replacement for #2687. It does not fully depend on any other PR, but is recommended to use with #2794, #2803, MycroftAI/mycroft-skills-manager#88 MycroftAI/mycroft-skills-manager#90 to stop Mycroft doing anything in directories it might not have access to anymore.

Description

This PR modifies bin/mycroft-start and bin/mycroft-stop to allow starting Mycroft services no matter where they installed and where the scripts itself are located. These are intended to be the main way to launch and run Mycroft for end-users, instead of them requiring to basically setup a full dev environment.

For end users this makes running dev_setup.sh unnecessary, unless they want to actually develop for Mycroft. In theory a PyPi package should also be possible to create now so users can just run pip install mycroft-core and have it work, but that might be something for later.

It also modifies the README to include new installations instructions using setup.py.

How to test

  1. Create a new empty virtual environment: python3 -m venv .venv.
  2. Run python3 setup.py install
  3. Run mycroft-start debug and observe everything be launched like normal
  4. Run mycroft-stop and observe everything being stopped like normal
  5. In a new terminal (outside of a virtual environment) run python3 setup.py install
  6. Run mycroft-start debug and observe everything be launched like normal
  7. Run mycroft-stop and observe everything being stopped like normal

Although testing the user installation (outside of virtual environment) should be enough, a system-wide installation can also be tested by running sudo python3 setup.py install.

Contributor license agreement signed?

CLA [x]

@devops-mycroft devops-mycroft added the CLA: Yes Contributor License Agreement exists (see https://github.com/MycroftAI/contributors) label Jan 15, 2021
@devops-mycroft
Copy link

devops-mycroft commented Feb 11, 2021

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

@devops-mycroft
Copy link

devops-mycroft commented Feb 12, 2021

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

2 similar comments
@devops-mycroft
Copy link

devops-mycroft commented Feb 12, 2021

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

@devops-mycroft
Copy link

devops-mycroft commented May 26, 2021

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

@PureTryOut
Copy link
Contributor Author

I've updated the launch script to log to the new $XDG_STATE_HOME as defined in the updated XDG Base Directory specification.

@devops-mycroft
Copy link

devops-mycroft commented Jun 12, 2021

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

1 similar comment
@devops-mycroft
Copy link

devops-mycroft commented Jun 12, 2021

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

@ChanceNCounter
Copy link
Contributor

Are system installs desirable? I'm really asking, not being sarcastic. Makes me nervous.

@PureTryOut
Copy link
Contributor Author

I mean, that's literally how people have it installed now. /opt is a system-wide install. Now it just allows Mycroft.ai to ship via Pip which allows user to easily update and stuff.

I think you're more worried about distribution packaging which is fair, but not the point of this PR (although this does help that).

@ChanceNCounter
Copy link
Contributor

ChanceNCounter commented Jun 13, 2021

/opt is a system-wide (edit: skill) install, but other bits aren't always. I'm more wondering how this would affect, for example, user-level config.

@PureTryOut
Copy link
Contributor Author

I'm not sure I understand what you mean? No matter where mycroft is installed, (using the XDG PR's) the configuration will always be read from the same locations. It doesn't matter if you have a system or a local install.

@ChanceNCounter
Copy link
Contributor

If I log out of my system, log in as a different user, and restart Mycroft, what do I encounter?

Is the device paired to the same account? Does it use the same Skill preferences? Does that mean it's using the same API keys and etc.? Now we need users who put Mycroft on the family PC to understand what's private and what's shared, so they can make sure they put settings in the right place.

@PureTryOut
Copy link
Contributor Author

I'm not sure how this PR makes that any different from before.

With Mycroft installed to /opt like usual, what happens if you log out of your system and log into a different user? Whatever happens there, will happen with this PR and a system-wide setup. I just fail to see how this PR changes such behaviour.

@ChanceNCounter
Copy link
Contributor

Under the current model, most of a given Mycroft build lives where it lives. /opt/mycroft is largely an exception, and XDG compliance only changes that to a certain extent, given that some config can, should, and will remain in a user's home folder.

@PureTryOut
Copy link
Contributor Author

I'm afraid I still don't understand what you're getting at. Are you saying that even though data and configs are in /opt/mycroft, the code and the executables itself are wherever the user git clone'ed mycroft-core to? I'm not sure how moving mycroft-core to a system-wide directory would break or mess things up.

With a system-wide installation, Mycroft still runs as your user reading the usual config locations (e.g. /etc/mycroft and ~/.config/mycroft). It also stores it's (temporary) data in your user folder still. So if you start Mycroft, then log out and log in as a different user and start Mycroft, it'll just run as 2 separate processes each acting as if no other Mycroft process is running, both storing their changed settings and data to their own user's folder. That is, if this is merged after the XDG PR's are in (#2794 and #2803) which I recommended in the original comment. If this is merged with /opt still in use then it will definitely mess things up when you run it as 2 different users at the same time yes.

This basically just removes the need of having duplicated files (the mycroft-core code) on your system if you want to use it with multiple users, and will allow easy installation via e.g. Pip.

@devops-mycroft
Copy link

devops-mycroft commented Jul 9, 2021

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

@PureTryOut
Copy link
Contributor Author

PureTryOut commented Nov 4, 2021

A lot of messing around later, CI now succeeds (except for VK but I doubt that's related to this PR) 🎉

This should be good to go now.

@codecov-commenter
Copy link

Codecov Report

Merging #2802 (9596c27) into dev (fd12c88) will not change coverage.
The diff coverage is n/a.

❗ Current head 9596c27 differs from pull request most recent head eef76c2. Consider uploading reports for the commit eef76c2 to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##              dev    #2802   +/-   ##
=======================================
  Coverage   53.04%   53.04%           
=======================================
  Files         123      123           
  Lines       11170    11170           
=======================================
  Hits         5925     5925           
  Misses       5245     5245           

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 fd12c88...eef76c2. Read the comment docs.

Copy link
Contributor

@krisgesling krisgesling left a comment

Choose a reason for hiding this comment

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

Hey I like the changes here, I think it's just the README that I think we need to work through.

At a foundational level, I don't think we should consider these "non-development setups". The only non-development installations that exist (in my opinion) are the Mark 1 and the Mark II.

I'm extremely cautious about pointing people to distro packaging too. We haven't had a great experience of it in the past which I'm sure you've heard about. Clearly you're the big exception, however we still have no visibility over how this is managed, how updates are made, and what the final experience looks like for users. Eg if I'm a Ubuntu user, my first inclination is to try apt install it, Ubuntu instead points to the Snap. I install the Snap and I have a horrible experience. It was built by the community with the best intentions and we took a quick stab at bringing it up to speed but it needs time and focus to get it there. In the end all we get are pissed off people that don't take another look at the project.

I'm sure Alpine is much better, however I can't imagine that it's a polished desktop experience because that just doesn't exist yet. Desktop hasn't been a focus for us because it requires a complete UX design process, and as you know our attention is on the Mark II right now. You can use the basic functions of Mycroft on a PC (or a phone) but it's setting up expectations with users that it's intended for those platforms, and that's just not the case yet.

This is still great stuff, and it should help us to work with distro maintainers and other people integrating Mycroft for us to release complete products in the future. I just wanted to be clear why I don't want us to present the software as a "ready-to-use product" in this readme.

README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
dev_setup.sh Outdated Show resolved Hide resolved
@PureTryOut
Copy link
Contributor Author

At a foundational level, I don't think we should consider these "non-development setups". The only non-development installations that exist (in my opinion) are the Mark 1 and the Mark II.

That's a shame, I think Mycroft has a lot of potential running on phones and desktops as well.

I'm extremely cautious about pointing people to distro packaging too. We haven't had a great experience of it in the past which I'm sure you've heard about. Clearly you're the big exception, however we still have no visibility over how this is managed, how updates are made, and what the final experience looks like for users. Eg if I'm a Ubuntu user, my first inclination is to try apt install it, Ubuntu instead points to the Snap. I install the Snap and I have a horrible experience. It was built by the community with the best intentions and we took a quick stab at bringing it up to speed but it needs time and focus to get it there. In the end all we get are pissed off people that don't take another look at the project.

I get that 100%. It's a shame these things happened in the past and it makes distributions look bad. I like to think not every distribution is like that but even some big names like openSUSE still have a very old version packaged (18.8.13 in their rolling distribution!) and don't even respond to emails asking about it.

I suppose as a project you don't have to tell the user about any distro packaging at all. You could provide a PyPi package which you maintain yourself (and thus have full control and insight over!), and if the user wants to get it from distro packaging instead they can look for it for themselves.

I'm sure Alpine is much better, however I can't imagine that it's a polished desktop experience because that just doesn't exist yet. Desktop hasn't been a focus for us because it requires a complete UX design process, and as you know our attention is on the Mark II right now. You can use the basic functions of Mycroft on a PC (or a phone) but it's setting up expectations with users that it's intended for those platforms, and that's just not the case yet.

I get that your priorities are elsewhere but I already consider Mycroft quite usable on desktop, especially in combination with the widget provided by KDE Plasma. You don't have to promote it, but I wouldn't completely dismiss it either 😉

This is still great stuff, and it should help us to work with distro maintainers and other people integrating Mycroft for us to release complete products in the future. I just wanted to be clear why I don't want us to present the software as a "ready-to-use product" in this readme.

That's fine! I'm happy your not dismissing it and I hope you're fine with me improving the situation for Alpine Linux/postmarketOS users wherever I can at least!

I applied the changes you suggested so this should be ready to go!

@krisgesling krisgesling changed the title Re-do non-development setups, allow system-wide and user installations rather than just virtual environments Allow system-wide and user installations rather than just virtual environments Jun 8, 2022
@krisgesling
Copy link
Contributor

Hey, I certainly don't think people shouldn't run Mycroft in other places. I agree there is huge potential there. It's just a question of what we consider a shipable product that we promote to non-techies.

Back to this PR though, I gave it a test run and hit three issues.

  1. Looks like we need to update the default log location in at least one more spot:
mycroft/client/text/__main__.py

This is what gets called when you run the CLI client.

  1. When stopping services I get:
mycroft-core/.venv/bin/mycroft-stop: 53: kill: Illegal option -S
  1. If using a bare mycroft-stop you get a shift error. Now is requiring mycroft-stop all

Are you seeing the same?

@PureTryOut
Copy link
Contributor Author

Looks like we need to update the default log location in at least one more spot:

Good call, done.

mycroft-core/.venv/bin/mycroft-stop: 53: kill: Illegal option -S

Strange, that option isn't used anymore (it was in the past but replaced for -SIGINT due to afaik Debian's default kill not supporting it). It was however present still in stop-mycroft.sh, where I replaced it now.

If using a bare mycroft-stop you get a shift error. Now is requiring mycroft-stop all

I'm not seeing the same no, for me this works just fine. Can you post the error you're getting?

@pep8speaks
Copy link

pep8speaks commented Jun 8, 2022

Hello @PureTryOut! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2022-06-08 08:07:24 UTC

@PureTryOut
Copy link
Contributor Author

@krisgesling it's been a few months again... Could you either approve/merge this or tell me what to do to get it merged?

@krisgesling
Copy link
Contributor

Hi Bart, this one falls in the same category as #2803

What this really needs is someone to comprehensively test it out and stake their reputation on it being solid. Then we can merge it in.

If any core contributor community members have time to review and test this it would be much appreciated.

@forslund
Copy link
Collaborator

I can try to make some time this weekend and during next week to test.

@PureTryOut
Copy link
Contributor Author

Well I'm willing to stake my reputation on it being solid, but I'm not a core contributor 😜

Thanks for the offer @forslund! Please let me know when you need something from me, I'm of course available on Mattermost.

@forslund
Copy link
Collaborator

Not sure I'm a core community contributor anymore but I should be able to do some tests :)

@krisgesling
Copy link
Contributor

Thanks Ake!

I would consider you both core community contributors, and Bart - if it was someone else's PR I'd definitely accept your review for a merge. I just believe that it's good practice to have someone other than the original author review and test PRs. Their fresh perspective can often see an assumption that we made or exercise it in a slightly different way than we expected.

bin/mycroft-stop Outdated Show resolved Hide resolved
Copy link
Collaborator

@forslund forslund left a comment

Choose a reason for hiding this comment

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

Ugh, the SIGINT signal name doesn't seem to be the correct one for me, sorry. It seems atleast for dash sh (Ubuntu 20.04) it needs to be -s INT for it to work properly... (same as current stop-mycroft.sh) Not sure if it's an issue with my setup or if -s INT is the most compatible argument...

Also saw some issues when forgetting to add arguments to the mycroft-start/stop scripts...I don't think they're a big issue but might be nice.

bin/mycroft-stop Outdated Show resolved Hide resolved
bin/mycroft-start Outdated Show resolved Hide resolved
bin/mycroft-stop Outdated Show resolved Hide resolved
forslund
forslund previously approved these changes Oct 28, 2022
Copy link
Collaborator

@forslund forslund left a comment

Choose a reason for hiding this comment

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

The blocker with the kill command in the stop script has been resolved. There still is the issue with passing no arguments to the start / stop scripts but I don't think it's really a blocker.

…stalled setups

These scripts will work with system-wide setups
(/usr/bin/mycroft-start), user setups (~/.local/bin/mycroft-start) and
virtual environments (.venv/bin/mycroft-start).

A big benefit is that they don't care where they're installed. They'll
launch from a virtual environment, user setups or system-wide setup, in
that order.

Also make sure these scripts are actually installed by setup.py.

These changes should make it possible to create a PyPi package
installable with pip.
@PureTryOut
Copy link
Contributor Author

I fixed that too like you suggested, should be good to go!

@PureTryOut
Copy link
Contributor Author

@krisgesling it's ack'ed by @forslund, should be good to go now.

@PureTryOut PureTryOut closed this by deleting the head repository Mar 22, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
CLA: Yes Contributor License Agreement exists (see https://github.com/MycroftAI/contributors)
Projects
No open projects
Status: Work in progress by author
Development

Successfully merging this pull request may close these issues.

mycroft: setup.py, mycroft.service,mycroft-cli-client and everything else is a dumpster fire
7 participants