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

Flag comparisons #3909

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

Conversation

joolswills
Copy link
Member

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 #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 ?

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.
Replace GCC version checks in depends_* functions with variable comparisons in flags.
The scriptmodule requires Python2 and python modules that are no longer packaged.
@cmitu
Copy link
Contributor

cmitu commented Apr 24, 2024

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:

  • it's easier to read and less error prone to write. Not a big issue in itself, since this kind of filtering is an exception rather than the norm for most modules.
  • a method can do more complex calculation (i.e. check a package version, OS version, etc.), though right now (for the modules considered in this PR or the previous one) it's not really needed. Comments can be added to the method to understand why the exception/filtering is needed.

@joolswills
Copy link
Member Author

joolswills commented Apr 24, 2024

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

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

cmitu commented Apr 25, 2024

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.

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

Successfully merging this pull request may close these issues.

None yet

2 participants