Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from

Conversation

erenfro
Copy link

@erenfro erenfro commented May 30, 2023

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
Copy link
Author

erenfro commented May 30, 2023

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
Copy link
Contributor

rasa commented May 30, 2023

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
Copy link
Author

erenfro commented May 30, 2023

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
Copy link
Author

erenfro commented May 30, 2023

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
Copy link
Contributor

rasa commented May 31, 2023

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

@erenfro
Copy link
Author

erenfro commented May 31, 2023

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

@rasa
Copy link
Contributor

rasa commented May 31, 2023

@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
Copy link
Member

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
Copy link
Author

erenfro commented Jun 5, 2023

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
Copy link
Member

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

@TheLocehiliosan
Copy link
Member

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.

@TheLocehiliosan TheLocehiliosan self-assigned this Jul 8, 2023
@rasa
Copy link
Contributor

rasa commented Jul 9, 2023

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.

TheLocehiliosan added a commit that referenced this pull request Oct 20, 2023
TheLocehiliosan added a commit that referenced this pull request Oct 20, 2023
This reverts commit fdfff94, reversing
changes made to 76ce3de.
@TheLocehiliosan TheLocehiliosan changed the base branch from master to develop November 12, 2023 13:55
@erenfro
Copy link
Author

erenfro commented Apr 7, 2024

@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
Copy link
Contributor

rasa commented Apr 17, 2024

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 .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants