Add ability to differentiate group ownership from user ownership.

[gderber]

I have a non-standard setup where the primary group name is not the same as the username. I was getting errors with chmod group not found. This patch fixes that issue for me.

[cmitu]

Duplicate of https://github.com/RetroPie/RetroPie-Setup/pull/3577 ?

[gderber]

Looks like it is, the difference is I did this in keeping with @joolswills desired method.

"After a discussion with some other developers, we would prefer this to be implemented with a variable $group. eg doing group=$(id -ng) in retropie_packages.sh and then doing chown $user:$group. Primarily because it's a clearer more well known syntax than leaving the group name out of the chown."

comment here: https://github.com/RetroPie/RetroPie-Setup/pull/3577#issuecomment-1255310575

[gderber]

Add it looks like a dup of #3612.

Apparently this is a simi-popular fix. People keep submitting patches.

[joolswills]

Sorry, I never did get around to finalising my changes when I looked at this before.

Ideally I would like to not include another global with a "common" name that could be accidentally used in modules and isn't clear it's a special variable. My idea was to deprecate "user" and introduce two new variables __user and __group or similar.

Thanks for the PR. I am for the idea, but it's a change that affects a lot of code, and I would like to be sure about it.

@cmitu Thoughts ?

[gderber]

If you'd prefer __user and __group, I'll gladly add that. Or even retropie_user and retropie_group or something along those lines.

What other changes were you thinking of?

One other oddity I noticed before doing this, and admittedly I haven't rebuilt everything yet, in addition to my main user that has a different group name from the username, the system also has a pi:pi user/group.

I haven't dug into it fully, but I did notice there are multiple files/folders owned by pi:pi in /opt/retropie. The pi user has NEVER run retropie_setup, or heen involved with retropie on the system in any way.

I am not at my computer at the moment, but it occurred to me I should also look for some chown pi:pi lines to change.

One change I did consider, but decided it'd be a much more difficult task was to create a helper function "setPermissions" that handles all chown actions.

The basic being:

function setPermissions () {
  local path="$1"
  if [[ -d "$path" ]]; then
    chown -R user:group "$path"
  else
    chown user:group "$path"
  if
}

Then usage would simply be setPermissions $path

Could potentially add sanity checks based on the parent directory.

[cmitu]

@cmitu Thoughts ?

Shorter is better, so if we do the change, then $__user, $__group should be ok.

@gderber I haven't dug into it fully, but I did notice there are multiple files/folders owned by pi:pi in /opt/retropie. The pi user has NEVER run retropie_setup, or heen involved with retropie on the system in any way.

Which files/folders ? We can check if the packages adding those folders have an issue.

.. One change I did consider, but decided it'd be a much more difficult task was to create a helper function "setPermissions" that handles all chown actions.

I thought we had something like that, but we only have 'mkUserDir' and moveConfigDir/moveConfigFile which apply the permissions on create/move. IMHO a simple chown is enough and is more explicit. Of course, this doesn't handle other checks but I don't think we care enough for them in that context.

EDIT: For older scripts/3rd party scripts compatibility, we have to keep $user available, at least temporary.

[gderber]

@cmitu Got it! I'll change the names to $__user:$__group

As for the permissions issue, it's something I got to run down. It does only seem to be affecting one script now that I am back at my computer. It's a custom script I am working on. I only brought it up because I thought it might be related to this in some way. The script that doesn't call chown anywhere in it which is why I thought it might be related to this. You can look at the current version here: https://github.com/hearthminion/RetroPie-Extra/blob/feature/ut/scriptmodules/ports/ut.sh

The permissions from that script result in /opt/retropie/ports/ut looking like:

drwxrwxr-x 1 pi   pi     32 Dec  9  2023 Help/
-rw-rw-r-- 1 pi   pi   138K Dec  9  2023 LICENSE.md
lrwxrwxrwx 1 root root   44 Jul 17 08:44 Maps -> /home/HOME/geoff/RetroPie/roms/ports/ut/Maps/
lrwxrwxrwx 1 root root   45 Jul 17 08:44 Music -> /home/HOME/geoff/RetroPie/roms/ports/ut/Music/
-rw-r--r-- 1 root root   32 Jul 17 08:44 retropie.pkg
lrwxrwxrwx 1 root root   46 Jul 17 08:44 Sounds -> /home/HOME/geoff/RetroPie/roms/ports/ut/Sounds/
drwxrwxr-x 1 pi   pi    466 Jul 17 08:44 System/
drwxrwxr-x 1 pi   pi    600 Dec  9  2023 System64/
drwxrwxr-x 1 pi   pi     60 Dec  9  2023 SystemLocalized/
lrwxrwxrwx 1 root root   48 Jul 17 08:44 Textures -> /home/HOME/geoff/RetroPie/roms/ports/ut/Textures/
drwxrwxr-x 1 pi   pi    854 Dec  9  2023 Web/

If it's somehow related to how retropie is setting permissions, I'd like to fix it here. If not, then it's off topic and we can ignore it.

[cmitu]

As for the permissions issue, it's something I got to run down. It does only seem to be affecting one script now that I am back at my computer. It's a custom script I am working on. I only brought it up because I thought it might be related to this in some way. [...]

The permissions are inherited from the archive downloaded, which is expanded in $md_inst. The pi:pi user/group appear because of the uid/gid (1000:1000) associated with the files in the archive.

EDIT: Btw, your script for UT adds some broken symlinks in $md_inst, pointing to non-existent folders under $romdir/ports/ut/.

[gderber]

@cmitu Got it! Thank you. As for the symlinks, installing game data is a matter of copying those folders from their source into $romdir/ports/ut.

[gderber]

As I am working on this, I've noticed there is different formatting for the various chown lines.

chown $__user:$__group chown "$__user:$__group" chown "$__user":"$__group"

chown -R $__user:$__group chown $__user:$__group -R

etc.

Would you like me to standardize these and if so what format would you like?

[cmitu]

We should use

chown -R "$__user":"$__group"

[gderber]

I'm still testing the latest commit. This is where I am at so far.

[gderber]

A re-install of everything I already had installed worked. I'm currently trying to install everything for further testing.

[gderber]

At this point most everything built and installled okay, with the permissions set as I expected.

What didn't install okay seems to be unrelated issues specific to those scripts. ResidualVM for example crashes with

engines/stark/gfx/driver.cpp: In static member function ‘static Stark::Gfx::Driver* Stark::Gfx::Driver::create()’:
engines/stark/gfx/driver.cpp:43:13: error: ‘OpenGLContext’ was not declared in this scope
   43 |         if (OpenGLContext.shadersSupported) {
      |             ^~~~~~~~~~~~~
make: *** [Makefile.common:156: engines/stark/gfx/driver.o] Error 1
make: *** Waiting for unfinished jobs....
strip: '/home/{{ username }}/Documents/Development/RetroPie-Setup/tmp/build/residualvm/residualvm': No such file
Could not successfully build residualvm - ResidualVM - A 3D Game Interpreter

Not sure what other tests you'd like done.

Note: I figured the failing scripts fall outside of the scope of this PR, which is why I didn't go into more detail.

[joolswills]

Thank you for all the testing. I will have a proper look at this as soon as possible.

[gderber]

It's not needed for the __user var come to think of it. I added it because I needed it when setting the __group var. I needed it for the group var becasue I didn't have anything along the lines of __group="$SUDO_GID" above.

A quick test I modified that file to read:

# if __user is set, try and install for that user, else use SUDO_USER
if [[ -n "$__user" ]]; then
    user="$__user"
    if ! id -u "$__user" &>/dev/null; then
        echo "User $__user not exist"
        exit 1
    fi
else
    user="$SUDO_USER"
    __user="$SUDO_USER"
    [[ -z "$user" ]] && user="$(id -un)"
    [[ -z "$__user" ]] && __user="$(id -un)"
    [[ -z "$__group" ]] && __group="$(id -gn)"
fi

echo $__user
echo $__group

exit

And when I ran sudo ./retropie_setup.sh

The results were: geoff root

With:

# if __user is set, try and install for that user, else use SUDO_USER
if [[ -n "$__user" ]]; then
    user="$__user"
    if ! id -u "$__user" &>/dev/null; then
        echo "User $__user not exist"
        exit 1
    fi
else
    user="$SUDO_USER"
    __user="$SUDO_USER"
    [[ -z "$user" ]] && user="$(id -un)"
    [[ -z "$__user" ]] && __user="$(id -un)"
    [[ -z "$__group" ]] && __group="$(id -gn $SUDO_USER)"
fi

echo $__user
echo $__group

exit

I get

geoff
geoff_g

Looking at it some more, do we even need the lines:

user=$SUDO_USER
__user=$SUDO_USER

at all? Simple cut out those lines completely?

I'm beginning to think the better code might be:

# if __user is set, try and install for that user, else use SUDO_USER
if [[ -n "$__user" ]]; then
    user="$__user"
    if ! id -u "$__user" &>/dev/null; then
        echo "User $__user not exist"
        exit 1
    fi
else
    [[ -z "$user" ]] && user="$(id -un $SUDO_USER)"
    [[ -z "$__user" ]] && __user="$(id -un $SUDO_USER)"
    [[ -z "$__group" ]] && __group="$(id -gn $SUDO_USER)"
fi

Or even $SUDO_UID and $SUDO_GID.

Thoughts?

[joolswills]

Thanks. As we check if the user exists we can also check if the group exists. Maybe something like this:

# if no user is specified
if [[ -z "$__user" ]]; then
    # get the calling user from sudo env
    __user="$SUDO_USER"
    # if not called from sudo get the current user
    [[ -z "$__user" ]] && __user="$(id -un)"
fi

# check if the user exists
if [[ -z "$(getent passwd "$__user")" ]]; then
    echo "User $__user does not exist."
    exit 1
fi

# if no group is specified get the users primary group
if [[ -z "$__group" ]]; then
    __group="$(id -gn "$__user")"
fi

# check if the group exists
if [[ -z "$(getent group "$__group")" ]]; then
    echo "Group $__group does not exist."
    exit 1
fi

# backwards compatibility
user="$__user"

[gderber]

I've made that change on my machine. I'm doing a rebuild to verify it works as expected. I do like it better than what was there before.

[gderber]

Looks like all the permissions match what I was expecting with what I have rebuilt (which is most, but not everything)

[joolswills]

Thank you for testing. I'm currently away on a work trip but will merge when I get back (and catch up on other RetroPie tasks).

[joolswills]

Thank you!

[gderber]

You're welcome! Thank you!

[joolswills]

I have reverted part of this change in 18c2b43000ec54d74ba016e0b8de654122377dba

Those files are not part of "RetroPie-Setup" itself but are installed by it - the "user" variable is set at the top. To work with the new "group" option a new group variable will need to be added to the files and set from within the usbromservice RetroPie-Setup module.

[gderber]

Sorry about that.

[joolswills]

No probs! I will make some additional changes to add the group variable to these external scripts.