Botspot / vdesktop

Run a second instance of Raspbian inside Raspbian.
GNU General Public License v3.0
125 stars 21 forks source link

Move config file out of source control #18

Closed mtlynch closed 3 years ago

mtlynch commented 3 years ago

Generally, files intended to be user-editable should not be under source control. Otherwise, a developer who wants to contribute upstream has to always remember to exclude their config file from their git commit.

Additionally, it's difficult to run vdesktop multiple times in a script with different configurations.

For example:

sudo ./vdesktop myimg.img cli-login # Log in as pi user
#TODO: Edit config file to change user setting...
sudo ./vdesktop myimg.img cli-login # Log in as a new user

The TODO is difficult to implement with standard bash utilities.

Proposal 1: Configure vdesktop through environment variables

The simplest approach would be to simply use environment variables. For example, we change the username line to this:

username="${VDESKTOP_USER-pi}"

Then changing users between script execution is straightforward and obvious:

sudo ./vdesktop myimg.img cli-login  # Log in as pi user
VDESKTOP_USER=newuser
sudo ./vdesktop myimg.img cli-login # Log in as a new user

Proposal 2: Specify config file on the command line

If we want to preserve the config file, we could allow the config file as a command-line option, e.g.

sudo ./vdesktop myimg.img cli-login --config=default.conf # Log in as pi user
sudo ./vdesktop myimg.img cli-login --config=newuser.conf # Log in as a new user

We could rename config to default.conf.example and instruct users to cp default.conf.example default.conf to use it. If no --config is specified, vdesktop uses default options.

The downside of this approach is that the existing config file format is very brittle. Options have to appear in a rigid order or everything breaks silently, as there's no rigid validation. It's also not friendly to changes over time, as additions or deletions of config file options will break all previous config scripts.


I'm willing to implement these changes if you agree with one of these proposals.

Botspot commented 3 years ago

Generally, files intended to be user-editable should not be under source control. Otherwise, a developer who wants to contribute upstream has to always remember to exclude their config file from their git commit.

Additionally, it's difficult to run vdesktop multiple times in a script with different configurations.

You make a very good point right there.

The simplest approach would be to simply use environment variables.

username="${VDESKTOP_USER-pi}"

Environment variables is a great idea. I think we'll go with that.

The downside of this approach is that the existing config file format is very brittle. Options have to appear in a rigid order or everything breaks silently, as there's no rigid validation. It's also not friendly to changes over time, as additions or deletions of config file options will break all previous config scripts.

The config file is a bit fragile. It gets its config options based on line numbers. Line 1 enables/disables the sleep infinity, line 2 specifies the username, etc.
If you were to set line 1 to "" (nothing, just a newline), vdesktop wouldn't care. But if you completely removed line 1, including the newline, then things would get messed up. But as long as you don't do that you're fine.
Adding new entries to the end of the file doesn't break anything either.

mtlynch commented 3 years ago

Environment variables is a great idea. I think we'll go with that.

Okay, cool. Would you like me to create a PR or is this something you'd prefer to do yourself?

Botspot commented 3 years ago

I already did it. :) Locally, at least. Haven't pushed the changes yet.

Botspot commented 3 years ago

Having a small issue here. This does not work:

export VDESKTOP_USER=1234
sudo /home/pi/vdesktop/vdesktop /path/to/my.img

This does not work:

VDESKTOP_USER=1234 sudo /home/pi/vdesktop/vdesktop /path/to/my.img

In both above cases vdesktop thought the variable was empty.

But this does work:

sudo VDESKTOP_USER=1234 /home/pi/vdesktop/vdesktop /path/to/my.img

Any ideas?

mtlynch commented 3 years ago

Can you push your changes to a branch so I can take a look?

mtlynch commented 3 years ago

I forgot sudo needs to have --preserve-env to inherit env variables that are exported. The sudo VDESKTOP_USER=1234 /home/pi/vdesktop/vdesktop should work, though.

Botspot commented 3 years ago

Not necessary to push changes. The same issue is evident in this simple script named vartest.sh:

#!/bin/bash
echo "This should not be blank: $VDESKTOP_USER"

Tests:

$ export VDESKTOP_USER=pi
$ ./vartest.sh
This should not be blank: pi
$ sudo ./vartest.sh
This should not be blank:

Looks like sudo is breaking it somehow. Maybe if I run export with sudo the variable will be visible to root processes:

$ sudo export VDESKTOP_USER=pi
sudo: export: command not found
Botspot commented 3 years ago

The sudo VDESKTOP_USER=1234 /home/pi/vdesktop/vdesktop should work, though.

Yes, it did work.

mtlynch commented 3 years ago

Right, it needs to be:

sudo --preserve-env ./vartest.sh

or the shorthand:

sudo -E ./vartest.sh
Botspot commented 3 years ago

Right, it needs to be:

sudo -E ./vartest.sh

That doesn't seem very user friendly. 100% of vdesktop users are used to sudo ./vdesktop/vdesktop img.img, but to do environment variable config all those users will have to remember to run sudo -E from now on.
Seems hard to remember, and impossible for vdesktop to detect and warn about. Maybe it would be better to have a separate, non-sudo script which takes the environment vars and uses them to generate a config file, which vdesktop then uses.
So to change the settings, you'd run something like:

export VDESKTOP_USER=foobar
./vdesktop/refresh-config.sh
sudo ./vdesktop/vdesktop /path/to/my.img

An added benefit with that would be that customized settings persist between reboots, instead of forcing the user to redo their export commands every time.

mtlynch commented 3 years ago

Maybe it would be better to have a separate, non-sudo script which takes the environment vars and uses them to generate a config file, which vdesktop then uses. So to change the settings, you'd run something like:

export VDESKTOP_USER=foobar
./vdesktop/refresh-config.sh
sudo ./vdesktop/vdesktop /path/to/my.img

At that point, you might as well just have vdesktop source a hardcoded path of environment variables like:

#!/bin/bash

SETTINGS_FILE=settings.env
if [ -f "$SETTINGS_FILE" ]; then
  source "$SETTINGS_FILE"
fi

echo "user=$VDESKTOP_USER"

And then you'd change settings like:

$ echo 'VDESKTOP_USER=otheruser' > settings.env
$ sudo ./vdesktop
user=otheruser

But it would be more flexible if the user could specify the extra .env file from the command line like:

sudo ./vdesktop/vdesktop /path/to/my.img --env-file settings.env

Although, to me, that's more work than just doing sudo -E.

Botspot commented 3 years ago

Problems with sudo -E:

So I'm trying to find a way which is as easy as env-vars, permanent as config files, and flexible as CLI flags.

I'll check out the .env idea.

Botspot commented 3 years ago

Okay, env file implemented. I have yet to update the docs, but it's hopefully self-explanatory.

Note that if you export an environment variable, the env file will take priority. To prevent the env file from setting a certain var, simply comment out the line in the env file.

mtlynch commented 3 years ago

Okay, env file implemented. I have yet to update the docs, but it's hopefully self-explanatory.

Unfortunately, I've decided to disengage from this repo at this point.

I run this script in production as part of my business, so I can't inspect the source to reverse engineer undocumented breaking changes, especially when they're bundled with tons of other changes that are not explained in the source history.

Introducing uninspectable binaries into source control is also a dealbreaker for me.

Thanks for your work! I'll check back in on the project in the future to see if it's stabilized, and maybe I can merge my fork back in.

Botspot commented 3 years ago

Unfortunately, I've decided to disengage from this repo at this point.

Understand. This version includes many advancements for GUI usage. (like HW acceleration & support for other desktop environments)
For CLI usage, not much has changed at all.

Key differences:

I run this script in production as part of my business, so I can't inspect the source to reverse engineer undocumented breaking changes, especially when they're bundled with tons of other changes that are not explained in the source history.

I tried hard to not introduce any breaking changes for you, however if you encounter any please let me know.
The README will be updated in a few days.

Thanks for your work! I'll check back in on the project in the future to see if it's stabilized, and maybe I can merge my fork back in.

Thank you too, for your help, your suggestions, and your donations!
Vdesktop has exciting plans for the future, and I'm glad you've been a part of it.