TheLocehiliosan / yadm

Yet Another Dotfiles Manager
https://yadm.io/
GNU General Public License v3.0
4.91k stars 176 forks source link

Fixed issue #455-lowercasing checks on both sides #456

Open erenfro opened 1 year ago

erenfro commented 1 year ago

What does this PR do?

It allows differences between lsb_release and os-release to continue to work by simply comparing values of key in filename and value from detection in lowercase, gracefully allowing prior existing cases to work as they should.

What issues does this PR fix or reference?

455

430

Previous Behavior

Comparison was using lsb_release, which formalized output -OR- went to os-release which ID is always lowercase per spec, causing differences in results based on which method was used for detection. It also accounts for distro_family that is using ID_LIKE, or if not available would revert to ID for base distros.

New Behavior

Comparison lowercases the value provided from the alternative processor, and from the output of the distro detection, to provide consistent matching behavior that works, now and for all previous use-cases.

Have tests been written for this change?

No

Have these commits been signed with GnuPG?

Yes


Please review yadm's Contributing Guide for best practices.

erenfro commented 1 year ago

Hang on, this requires at least Bash 4.0 to work, so I may have to go with the more "POSIX" friendly version of lowercasing.

rasa commented 1 year ago

yadm doesn't currently have a dependency on tr, but it does on awk, so perhaps it would be better to use: "${AWK_PROGRAM[0]}" '{print tolower($0)}' instead of tr?

This also allows us to remove the unneeded pipe | operation, so value="$(echo $value | tr '[:upper:]' '[:lower:]')" becomes edited value=$("${AWK_PROGRAM[0]}" '{print tolower($0)}' <<<"${value}") and echo "$distro" | tr '[:upper:]' '[:lower:]' becomes edited "${AWK_PROGRAM[0]}" '{print tolower($0)}' <<<"${distro}"

erenfro commented 1 year ago

awk is viable as well, yes. I could also fix #430 as well, with a method I looked over, just a couple lines that looks at ID_LIKE and ID, which is literally a simple 3-line change. I'll make these changes and commit and let you see. :)

erenfro commented 1 year ago

Using the "${AWK_PROGRAM[0]}" '{print tolower($0)}' "${distro}" caused a surge of new issues with it trying to open files named various "fedora", "arch", "linuxmint", "opensuse", "pop" in my case, with "No such file or directory", so I reverted to the echo | awk method which is POSIX standard, and it's working.

rasa commented 1 year ago

Sorry, that should have read "${AWK_PROGRAM[0]}" '{print tolower($0)}' <<<"${distro}" <<< is supported by bash 3

erenfro commented 1 year ago

Done, tested, and verified. Thankfully I have a MacBook Pro with bash 3 handy to test with. :)

rasa commented 1 year ago

@erenfro I don't know why, but neither tr '[:upper:]' '[:lower:]' or tr '[[:upper:]]' '[[:lower:]]' work on OpenWRT's BusyBox v1.33.2 (2023-04-14 11:34:26 UTC) multi-call binary. Fortunately, awk's tolower() does work. Of course tr '[A-Z]' '[a-z]' works as well.

TheLocehiliosan commented 1 year ago

I like this change. I think using awk seems like the best route (@rasa thanks for the input about OpenWRT).

I think that maybe this needs to be broadened. With this change distro and distro_family are case insensitive. Perhaps this should be extended to: arch, os, hostname, user, and class. Would those also make sense to be case insensitive?

erenfro commented 1 year ago

Yep I could agree with that, arch, user, and class. I'll look into that today and see about updating the PR if you'd like. And also, do you want me to squash commits, (and if so, reference of how to do so if plausible, since I've only ever done that once LOL)

TheLocehiliosan commented 1 year ago

You don't have to squash the commits. The bigger challenge will be updating the tests. (as is always the case)

TheLocehiliosan commented 12 months ago

I've thought about this some more, and I've decided to create a Bash 3 compatible function that uses printf. Today, yadm doesn't have any actual Awk dependency unless you use the built in templates, and I'd like it to remain dependency free.

The basic approach will be to iterate over every char of the string, and determine the ascii value:

ascii_val=$(printf "%d" "'$char'")

Then only change the chars with values between 65 & 90 (adding 32 and converting them to lower case).

I think this should be good for these values, and not require Awk.

rasa commented 12 months ago

This works for me, and is bash 3 compliant:

lc() {
  local i=0 c
  while printf -v c %d "'${1:(i++):1}"; do 
    ((c)) || break
    ((c>64&&c<91)) && ((c+=32))
    printf "\x$(printf %x $c)"
  done
}

((${BASH_VERSINFO[0]}>3)) && function lc { printf %s "${1,,}"; }

and appears to work with non-ASCII (UTF-8) strings.

erenfro commented 2 months ago

@erenfro I don't know why, but neither tr '[:upper:]' '[:lower:]' or tr '[[:upper:]]' '[[:lower:]]' work on OpenWRT's BusyBox v1.33.2 (2023-04-14 11:34:26 UTC) multi-call binary. Fortunately, awk's tolower() does work. Of course tr '[A-Z]' '[a-z]' works as well.

Looks like, since yadm is using /bin/sh which means POSIX compliance, the only real POSIX compliant solution that seems to work globally is basically utilizing awk's tolower() method.

echo "$a" | awk '{print tolower($0)}'

As local is not POSIX compliant, and I can't tell for sure if using tr '[A-Z]' '[a-z]' is compliant since the standard is more on the use of tr '[:upper:]' '[:lower:]'.

Either way, this problem continues to hit me still to this day, dealing with working on various servers both ID=debian and ID_LIKE=debian, and this not following form. :)

rasa commented 2 months ago

Looks like, since yadm is using /bin/sh which means POSIX compliance…

@erenfro While the yadm command does begin with #!/bin/sh(for compatibility), it immediately executes exec bash here.

Note that yadm does not use any bash 4+ features, as bash 3 is often installed on MacOS. It doesn’t depend on /bin/sh, or POSIX compliance. See here.

As local is not POSIX compliant.

local is available in bash 3. See http://tiswww.case.edu/php/chet/bash/NEWS .