search

PapirusDevelopmentTeam / papirus-folders

a script that lets you change the colors of folders in Papirus icon theme
https://git.io/papirus-folders
MIT License
600 stars 32 forks source link

Check for $USER #9

Closed m-roberts closed 5 years ago

m-roberts commented 5 years ago

When determining user, check for $USER. Without this, an issue can occur in environments that do not correctly set LOGNAME.

SmartFinn commented 5 years ago

Without this, an issue can occur in environments that do not correctly set LOGNAME.

In this case, I think replacing $LOGNAME variable to $(logname) command is the better way.

$ > env LOGNAME=fakeuser logname
sergei
$ >

Adding $USER variable is useless because that command does the same https://github.com/PapirusDevelopmentTeam/papirus-folders/blob/master/papirus-folders#L171

The id command is better because it works even $USER unset: 

$ > env USER= id -nu
sergei
$ >
m-roberts commented 5 years ago

The issue scenario that I am dealing with, is installing Papirus to a currently-being-assembled OS image and applying this patch to it. That is, I am on a host machine with a username not found on the OS image that I am creating. The underlying infrastructure that I am basing my modifications on spoofs $USER, but not $LOGNAME or $SUDO_USER. I have added these in to the build system that I am currently using (as a temporary fix), but it is not desirable in this form.

Another option that would suit the requirements of my scenario would be if it were possible to specify the user as a command line argument (or similar), rather than purely determining it from the script. This would allow me to run this script with chroot on a host machine.

Perhaps it would be beneficial to discuss this use-case further with you to determine the most appropriate solution? This PR was simply an attempt to solve it quickly for me, but I think that a better solution is likely to exist that will make it more flexible - particularly in instances where it is not actually the 'current user' who is configuring Papirus.

SmartFinn commented 5 years ago

I decided to remove $LOGNAME support, but I still can understand how it can break the script in a chroot environment. I tested it and anything works as expected.