zypper.packages lowercases package names?

swsnr commented 2 years ago

Describe the bug

With the following operation

zypper.packages(
    name='Install Firefox from Suse repos',
    packages=['MozillaFirefox'],
    present=True,
)

pyinfra apparently attempts to install mozillafirefox, that is, the lowercase name:

[pyinfra-testing] >>> sudo -H -n -E sh -c 'export REDACTED && zypper --non-interactive install -y mozillafirefox'
[pyinfra-testing] Loading repository data...
[pyinfra-testing] Reading installed packages...
[pyinfra-testing] 'mozillafirefox' not found in package names. Trying capabilities.
[pyinfra-testing] No provider of 'mozillafirefox' found.
[pyinfra-testing] - Did you mean MozillaFirefox?

That's just plain wrong; zypper has case-sensitive package names, so it really needs to install MozillaFirefox, not mozillafirefox.

To Reproduce

Run the operation above against a Suse system.

Expected behavior

Install MozillaFirefox exactly as spelled in the operation.

Meta

Include output of pyinfra --support.

pyinfra --support
--> Support information:

If you are having issues with pyinfra or wish to make feature requests, please
check out the GitHub issues at https://github.com/Fizzadar/pyinfra/issues .
When adding an issue, be sure to include the following:

System: Linux
  Platform: Linux-5.16.11-arch1-2-x86_64-with-glibc2.35
  Release: 5.16.11-arch1-2
  Machine: x86_64
pyinfra: v1.6.2
Executable: REDACTED/env/bin/pyinfra
Python: 3.10.2 (CPython, GCC 11.1.0)

sysadmin75 commented 2 years ago

My proposed fix just passes the lower parameter to the ensure_packages operation.

I personally believe the better solution is to change the default value of lower in ensure_packages should be False instead of True. However this is my opinion and probably requires a bigger discussion.

Fizzadar commented 2 years ago

I personally believe the better solution is to change the default value of lower in ensure_packages should be False instead of True. However this is my opinion and probably requires a bigger discussion.

I'm inclined to agree here, although a lot of package managers are case insensitive so there's probably no true correct solution here. But from a code perspective it seems cleaner to default to False and explicitly enable the lowercasing (https://github.com/Fizzadar/pyinfra/issues/772).

Thank you for the PR to fix this, will release in v1.7 later today!

Fizzadar commented 2 years ago

Now released in v1.7!

swsnr commented 2 years ago

Thanks for the fix.

But why lowercase at all? If I'd want lowercased names I could just call .lower() explicitly? Why so much magic? Wouldn't it be cleaner to just take the given package names as is?

Fizzadar commented 2 years ago

It dates back to 2015! https://github.com/Fizzadar/pyinfra/commit/ea437eeff89fac0274a4d709bee56e9606d31131

I think the idea was to normalise everything for case-insensitive package managers. I agree this is incorrect and that actually all package name lowercasing should be removed. Added https://github.com/Fizzadar/pyinfra/issues/779.

pyinfra-dev / pyinfra