Shell plugin Support API

[bfloch]

(Not sure if this qualifies as REP since it is quite small in scope, feel free to rename/tag)

Currently shell plugins are expected to available on specific platforms:

Windows -> cmd
Windows -> PowerShell
Windows -> Pwsh
Non-Windows -> bash
Non-Windows -> zsh
...

The logic for the platform is defined in the windows specific shells via if windows ...

We recently got more shell plugins which could be considered optional, but also we live in complex times with WSL, MacOS switching from bash to zsh etc. where it is hard to predict which shells will work on which platform.

I suggest we add two new pure abstract read-only classmethods to the Shell base class:

is_required
    States if Shell is required on current platform.
    This way we can still generate sensible exceptions in cases an expected shell is 
    not available.
    This might also help implementing more details logic like for example if a 
    platform deprecates and removes a shell we generate platform version specific
    results.

is_available
   States if Shell is available on current platform. In most cases this is just going through
   the implementation of the executable discovery, but it will not throw and states the 
   intend better for availability checks.

A platform-dependent required Shell can be now implemented like this

class Cmd(Shell):

@classmethod
def is_required(cls):
    # ...
    return platform_.name == "windows"

@classmethod
def is_available(cls):
    try:
        return cls.executable is not None
    except RuntimeError:
        return False

An optional platform can be implemented via:

class Zsh(SH):

@classmethod
def is_required(cls):
    # Pseudo code with sample versions
    return platform_.name = "macos" and platform_.os > "12.2"

@classmethod
def is_available(cls):
    try:
        return cls.executable is not None
    except RuntimeError:
        return False

The plugin registration now becomes uniformly:

def register_plugin():
    return _verify_shell(Zsh)

def _verify_shell(shell):
    if shell.is_available():
        return shell
    elif shell.is_required():
        raise RequiredShellNotAvailable(shell.name)
    return None

EDIT 1: Python version agnostic class properties are somewhat complicated. Sticking to class methods.

[nerdvegas]

Sure I don't have a problem with this, although I don't think it's a REP. One thing I'd say is that is_required is a little confusing, perhaps is_supported would be more intuitive. Thx A

…

On Wed, Aug 21, 2019 at 2:41 AM Blazej Floch ***@***.***> wrote: (Not sure if this qualifies as REP since it is quite small in scope, feel free to rename/tag) Currently shell plugins are expected to available on specific platforms: Windows -> cmd Windows -> PowerShell Windows -> Pwsh Non-Windows -> bash Non-Windows -> zsh ... The logic for the platform is defined in the windows specific shells via if windows ... We recently got more shell plugins which could be considered *optional*, but also we live in complex times with WSL, MacOS switching from bash to zsh etc. where it is hard to predict which shells will work on which platform. I suggest we add two new pure abstract properties to the Shell base class: is_required States if Shell is required on current platform. This way we can still generate sensible exceptions in cases an expected shell is not available. This might also help implementing more details logic like for example if a platform deprecates and removes a shell we generate platform version specific results. is_available States if Shell is available on current platform. In most cases this is just going through the implementation of the executable discovery, but it will not throw and states the intend better for availability checks. A platform dependent required Shell can be now implemented like this class Cmd(Shell): @propertydef is_required(cls): # ... return platform_.name == "windows" @propertydef is_available(cls): try: return cls.executable is not None except RuntimeError: return False An optional platform can be implemented via: class Zsh(SH): @propertydef is_required(cls): # Pseudo code with sample versions return platform_.name = "macos" and platform_.os > "12.2" @propertydef is_available(cls): try: return cls.executable is not None except RuntimeError: return False The plugin registration now becomes uniformly: def register_plugin(): return _regiser_shell(Zsh) def _register_shell(shell): if shell.is_available: return shell elif shell.is_required: raise RequiredShellNotAvailable(shell.name) return None — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#696>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAMOUSX565TCUIYRUOQQXC3QFQND5ANCNFSM4INZ2NQA> .

[bfloch]

Thanks for the feedback.
I'll add it as separate commit to my shell improvement branch.

[bfloch]

I renamed this issue to not confuse it with the shell enhancements PR (which does not yet implement the suggestion here, since I want to take some more time to think over it).

[bpabel]

It seems like if you're making is_available() and is_required() part of the api for the shell plugins, that the handling of those should be done by rez itself, not by the plugins, during registration.

[bfloch]

Thanks for the feedback.
Yes the "logic" that I described in "_verify_shell" should now be consistent for all shells and could be part of the registration.

The only issue I see is that now we would have special plugin code only for the shells.
That's why I stuck with the current model where the registered plugin figures out if/how to register.
Another benefit is that you could totally circumvent this logic if necessary - and this power is owned by plugin writers, not the API.

I don't have strong feelings about it but my proposal would be to instead make the verify_shell (better naming?) a standard method in rez.shells similar to create_shell, so that all register_plugin implementations are consistent in the default case.

[nerdvegas]

Revisiting this. So I'm going to implement part of this just now, because currently, rez-selftest will fail if you don't have one of the supported shells installed. This isn't really intuitive - if I don't have zsh installed for eg, I would expect those tests to be skipped.

To that end, I'll put in a PR which:

• adds an 'is_available' method to shells, as described above;
• update the 'shell_dependent' test decorator to only iterate over available shells.

A note on plugin registration: I think plugin registration shouldn't contain any logic beyond a simple platform check, where we know that the shell is only an option for that platform(s). Anything else (eg whether it's required, whether it's installed on the system) are runtime considerations.

[nerdvegas]

I'm going to close this as #747 largely addresses it. However if anyone feels that further aspects of this ticket need to be addressed, please feel free to open a new ticket, cheers!