RetroPie / RetroPie-Setup

Shell script to set up a Raspberry Pi/Odroid/PC with RetroArch emulator and various cores
Other
9.99k stars 1.38k forks source link

Flag comparisons #3909

Closed joolswills closed 1 month ago

joolswills commented 2 months ago

packages - allow enabling/disabling a module with comparison flags

rp_module_flags can now contain a variable comparison to enable or disable a scriptmodule. The comparison should be in the format :\$var:cmp:val or !:\$var:cmp:val

eg. :\$__gcc_version:-lt:7 would be evaluated as [[ $__gcc_version -lt 7 ]]

This would enable a module if the comparison was true or disable if !:\$__gcc_version:-lt:7 was used. Only global variables set before modules are loaded (eg via system.sh) are supported.

$ is escaped so variables are not evaluated when the module is sourced. It works without, but provides less useful information in the setup menus as the variable name will not be visible.

Convert modules to using variable comparisons in flags

Replace GCC version checks in depends_* functions with variable comparisons in flags.

mysticmine - Disable on Debian versions newer than 10 (buster)

The scriptmodule requires Python2 and python modules that are no longer packaged.


This is a rework of the functionality in the draft PR https://github.com/RetroPie/RetroPie-Setup/pull/3907 but using module flags over a hook function.

It's more compact, and covers most cases we need. It also has the advantage of providing some information for disabled modules in the setup gui.

@cmitu thoughts ?

cmitu commented 2 months ago

While it's shorter to use the module flags for this kind of comparison, I think I prefer the version with the _enable method since:

joolswills commented 2 months ago

Thanks for the feedback.

Before I revisited this and spent some more time on it I would have probably agreed but the _supported_* function is actually more limiting.

As the function is run after all the other flag logic, it can only add additional logic for disabling a module. The new method allows the logic to be used in any order with other flags (although in the current cases this isn't required). It also can be used to "enable" a module based on a comparison which the function method can't do (and it wouldn't be much use that way since it's processed last).

Comments can be added to the function, but if viewing a package in the setup an end user wouldn't get any information and the code would need to be checked.

So I still would like to include this method but we could include the function method if needed later.

I also didn't like the aspect of the function with the bash "reversed" true/false with 0/1. I think I went through about 5 different naming conventions to try and make it less confusing but it's not ideal. I think the flags is somewhat more intuitive in that respect.

joolswills commented 2 months ago

Also with the new method some logic could be added to the flag display in retropie_setup that could "decode" the comparison and so it would be clear why a module was disabled (eg showing the variable value vs the comparison). This wouldn't be possible with the function, unless we had some additional code in the function itself perhaps to return some information. So I think it has advantages that way.

cmitu commented 2 months ago

Thanks for the response. Didn't consider that running the _enabled function would be happening later on and wouldn't have the same effect. I guess the cons outweight the pros for using a function.