fix: Make symlink_ds.sh more robust by checking if /usr is a distinct mountpoint

[dylanmtaylor]

Pull request

Purpose This handles installation on immutable filesystems better. If /usr is not a distinct mountpoint, the existing check logic does not work as intended

Approach If /usr does not have its own distinct mountpoint, we use the immutability check on the root filesystem to determine if it is read only or not.

Open Questions and Pre-Merge TODOs Check all boxes as they are completed

• [x] Use github checklists. When solved, check the box and explain the answer.

Learning Describe the research stage N/A

Requirements Check all boxes as they are completed

• [x] These changes meet the standards for contributing.
• [x] I have read the code of conduct.

[nemchik]

I like where this is going, but I think we'll need to level up the regex in the grep. Looking at my output of findmnt -n / -o OPTIONS | grep -P "\bro\b" in wsl running Ubuntu (I know this might not be exactly the same as a VM or bare metal) I get the following output:

rw,relatime,discard,errors=remount-ro,data=ordered

The issue with the above is errors=remount-ro (which should mean the system is only ro if there are errors attempting to mount) would be caught by the current regex and prevent the symlink from being created.

I think this regex would work, but I'm open to something more optimized or cleaner/easier: ^ro$|^ro,|,ro,|,ro$

Another thought is, if we're checking the top level filesystem path for being ro and it is, and the user's home directory is not on a separate writable filesystem, then this would result in a failure. So we might be better off first determining if every possible location we could put a working symlink is writeable and then picking the best option from the usable ones, or leave the SYMLINK_TARGETS array empty if there are no writable locations. At least one of the following paths would need to be writable, and included in the user's PATH:

/usr/bin/
/usr/local/bin/
${HOME}/bin/
${HOME}/.local/bin/

And to be clear, I don't care if the path is on a ro filesystem or not. I care if the path is writable. Maybe somehow /usr is ro but /usr/local/bin is still writable for example. So some pseudo code here would be

SYMLINK_TARGETS=()
CHECK_PATHS=("/usr/bin/" "/usr/local/bin/" "${HOME}/bin/" "${HOME}/.local/bin/")
for CHECK_PATH in "${CHECK_PATHS[@]}"; do
    if [[ -d ${CHECK_PATH} && -w ${CHECK_PATH} && echo "${PATH}" | grep -P "${CHECK_PATH}" ]]; then
        SYMLINK_TARGETS=("${CHECK_PATH}/ds")
        break
    fi
done

And then de-nest the for SYMLINK_TARGET in "${SYMLINK_TARGETS[@]}"; do loop so that it isn't a loop because we would only be operating on one path (the first one we find that is writable and in the user's PATH), but also we would need to know if SYMLINK_TARGETS is empty, and if so, don't run the rest of the attempt to create the symlink, so it might be better to just lift that code into the if statement above instead of setting SYMLINK_TARGETS=("${CHECK_PATH}/ds"), just do the work right there. If we go that route (moving the symlink creation logic inside the above example) I would keep the break so that we don't create multiple symlinks, and then after that loop, check all of the locations in CHECK_PATHS to see if a symlink was actually created, and if not, then warn the user that there is no symlink and they will have to execute DS by running bash "${SCRIPTNAME}".

One last thing about the above: the check on PATH is crap and we should use something better, like splitting PATH on : into an array and then checking the array to contain an exact match for CHECK_PATH

Sorry this got long-winded, but the more I looked at it, the more I thought we may as well consider all possibilities

[dylanmtaylor]

I like where this is going, but I think we'll need to level up the regex in the grep. Looking at my output of findmnt -n / -o OPTIONS | grep -P "\bro\b" in wsl running Ubuntu (I know this might not be exactly the same as a VM or bare metal) I get the following output:

rw,relatime,discard,errors=remount-ro,data=ordered

The issue with the above is errors=remount-ro (which should mean the system is only ro if there are errors attempting to mount) would be caught by the current regex and prevent the symlink from being created.

I think this regex would work, but I'm open to something more optimized or cleaner/easier: ^ro$|^ro,|,ro,|,ro$

Another thought is, if we're checking the top level filesystem path for being ro and it is, and the user's home directory is not on a separate writable filesystem, then this would result in a failure. So we might be better off first determining if every possible location we could put a working symlink is writeable and then picking the best option from the usable ones, or leave the SYMLINK_TARGETS array empty if there are no writable locations. At least one of the following paths would need to be writable, and included in the user's PATH:

/usr/bin/
/usr/local/bin/
${HOME}/bin/
${HOME}/.local/bin/

And to be clear, I don't care if the path is on a ro filesystem or not. I care if the path is writable. Maybe somehow /usr is ro but /usr/local/bin is still writable for example. So some pseudo code here would be

SYMLINK_TARGETS=()
CHECK_PATHS=("/usr/bin/" "/usr/local/bin/" "${HOME}/bin/" "${HOME}/.local/bin/")
for CHECK_PATH in "${CHECK_PATHS[@]}"; do
    if [[ -d ${CHECK_PATH} && -w ${CHECK_PATH} && echo "${PATH}" | grep -P "${CHECK_PATH}" ]]; then
        SYMLINK_TARGETS=("${CHECK_PATH}/ds")
        break
    fi
done

And then de-nest the for SYMLINK_TARGET in "${SYMLINK_TARGETS[@]}"; do loop so that it isn't a loop because we would only be operating on one path (the first one we find that is writable and in the user's PATH), but also we would need to know if SYMLINK_TARGETS is empty, and if so, don't run the rest of the attempt to create the symlink, so it might be better to just lift that code into the if statement above instead of setting SYMLINK_TARGETS=("${CHECK_PATH}/ds"), just do the work right there. If we go that route (moving the symlink creation logic inside the above example) I would keep the break so that we don't create multiple symlinks, and then after that loop, check all of the locations in CHECK_PATHS to see if a symlink was actually created, and if not, then warn the user that there is no symlink and they will have to execute DS by running bash "${SCRIPTNAME}".

One last thing about the above: the check on PATH is crap and we should use something better, like splitting PATH on : into an array and then checking the array to contain an exact match for CHECK_PATH

Sorry this got long-winded, but the more I looked at it, the more I thought we may as well consider all possibilities

I like where your head is at with this - I'll try to update this in the near future to add more checks.

[dylanmtaylor]

I think this should be merged as-is as I'm hitting issues with it and prefactoredin the future if I get some time.

[CLHatch]

In an update I'm doing to add MacOS support, I've changed this to not even check the readonly status, and instead just attempt to create the symlink in each folder until it's successful. Correct me if I'm wrong, but wouldn't that be sufficient, silently failing until we're successful, and give an error if somehow none of the locations worked?

https://github.com/GhostWriters/DockSTARTer/blob/macos/.scripts/symlink_ds.sh

[CLHatch]

I went ahead and worked on the update I was working on and merged it separately. It should have no problems with readonly folders, and now checks the folders in the order listed in the PATH variable. https://github.com/GhostWriters/DockSTARTer/pull/2270