mviereck / x11docker

Run GUI applications and desktops in docker and podman containers. Focus on security.
MIT License
5.68k stars 379 forks source link

Please install stuff in `/usr/local/share` by default, not `/usr/share` #479

Closed volkertb closed 1 year ago

volkertb commented 1 year ago

Hi,

First of all: awesome project!

Just one small annoyance I noticed when I ran the x11docker script with the --update option: I noticed it installing (or at least trying to install) stuff under /usr/share. That really ought to be /usr/local/share, IMO. After all, I'm manually installing something outside of the distro's package manager.

I'm specifically referring to these log lines in the console output:

x11docker note: Storing README.md, CHANGELOG.md and LICENSE.txt in
  /usr/share/doc/x11docker

x11docker note: Storing man page in /usr/share/man/man1/x11docker.1.gz

gzip: skipping: x11docker.man does not exist
x11docker note: Error storing man page.

The log didn't report anything else being installed under /usr other than /usr/share, but obviously, this would apply to anything being installed under /usr: it should be /usr/local by default, unless the software is being installed through a distro-specific package manager.

See also https://refspecs.linuxfoundation.org/FHS_3.0/fhs/ch04s09.html

Thanks for considering.

mviereck commented 1 year ago

You are right that x11docker should use /usr/local following the specifications. However, there is one issue why I decided against it: If x11docker is installed in /usr/local/bin, it is not within PATH for root. This is at least true for debian, likely for other distributions, too. But root should be able to run x11docker, in case one does not want to use insecure group docker. So x11docker installs itself as /usr/bin/x11docker. I could store at least the docs in /usr/local/share (not sure if at least man would find the man page); but having the script in /usr, but the rest in /usr/local is somewhat inconsistent.

volkertb commented 1 year ago

Most people will use sudo (or doas) as a non-root user for operations that require root, in which case /usr/local/bin will be in the path. Only more experienced people will sometimes log in as root or use su to obtain a root shell, and those people tend to be aware of the absence of /usr/local/bin from the root path. They'll be savvy enough to prefix the path whenever they invoke x11docker as root. And even if they don't like prefixing the path, and don't mind their distro installation being cluttered, they'll be sure to specify --prefix=/usr during installation, to override the default.

So this does not seem enough of a justification to me to default to /usr/bin.

Or did I misunderstand your explanation?

mviereck commented 1 year ago

Or did I misunderstand your explanation?

You understand me right.

Most people will use sudo (or doas) as a non-root user for operations that require root, in which case /usr/local/bin will be in the path.

This seems to be true now. I've also checked su here an Debian bullseye, and it has /usr/local/bin in PATH, too. It seems this has changed meanwhile. My observation of missing /usr/local/bin has been about 5 years ago, when development of x11docker started. It was back then when I decided to use /usr/bin.

So you are right, x11docker should use /usr/local in general for all files. I just have to write a clean update procedure that does not break anything and leaves no files behind.

However, there might still be some distributions around without /usr/local/bin in PATH for su/sudo.

mviereck commented 1 year ago

I've published release v7.6.0 to adress this issue: https://github.com/mviereck/x11docker/releases/tag/v7.6.0

I hope this works well and does not cause too much confusion for users.

volkertb commented 1 year ago

Thanks for resolving this so quickly! I'm personally confident that this won't cause issues for the vast majority of users.

Thanks again for developing this tool and sharing it with the world. :slightly_smiling_face:

By the way, sorry for creating a duplicate issue. I could have sworn I did a search through existing issues before I created this one.