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

feat: Implement auto-detection of subexecutor #12261

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

pepoluan
Copy link
Contributor

@pepoluan pepoluan commented Mar 6, 2024

Standards checklist:

  • The PR title is descriptive.
  • The PR doesn't replicate another PR which is already open.
  • I have read the contribution guide and followed all the instructions.
  • The code follows the code style guide detailed in the wiki.
  • The code is mine or it's from somewhere with an MIT-compatible license.
  • The code is efficient, to the best of my ability, and does not waste computer resources.
  • The code is stable and I have tested it myself, to the best of my abilities.
  • If the code introduces new aliases, I provide a valid use case for all plugin users down below.

Changes:

  • Adds a new library, named 00subexecutor.zsh. The 00 part is to ensure that it runs first, because it will set a certain zstyle that is needed by other (patched) libraries.
  • Patches several other libraries formerly hard-coded to use sudo, to adapt to whatever subexecuter has been detected in 00subexecutor.zsh
  • Adds a new 'global' function called subex that can be used in place of (parameterless) sudo in all plugins, if we ever want to do the heavy-lifting of making all plugins have transparent support for non-sudo subexecutor

Other comments:

This should close #12245

This PR modifies the core parts of OMZ, it is DIFFERENT from #11395, which has been closed anyways.

@ohmyzsh ohmyzsh bot added Area: core Issue or PR related to core parts of the project Topic: alias Pull Request or issue regarding aliases labels Mar 6, 2024
Comment on lines +35 to +36
# The alias provides a 'hardcoded', invariant subexecutor to use throughout the shell session
alias _="$_SUBEX "
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is taken out from lib/misc.zsh because the definition of the alias now relies completely on what subexecutor is being used.

# cmd name only, or if this is doas/sudo or ssh, the next cmd
local _subex
zstyle -s ':omz' 'subexecutor' _subex
local CMD="${1[(wr)^(*=*|${_subex}|_|subex|mosh|rake|-*)]:gs/%/%%}"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I replaced sudo in the filter with ${_subex}|_|subex for the following reasons:

  • The subexecutor is not necessarily sudo now (can be doas, for instance)
  • There's an underscore _ alias (previously defined in lib/misc.zsh) that aliases the subexecutor
  • There's a new global function subex that also invokes the subexecutor

Comment on lines -30 to -32
## super user alias
alias _='sudo '

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved to 00subexecutor.zsh

@pepoluan
Copy link
Contributor Author

pepoluan commented Mar 6, 2024

Didn't edit CODEOWNERS file because I'm not sure if that file only applies to plugins, or can also cover a singular file such as lib/00subexecutor.zsh

Edit: Okay just found out how the bot reads the CODEOWNERS file, so I guess I'll assume responsibility for that one file. Commit incoming.

I willingly am taking up ownership and responsibility of the `lib/00subexecutor.zsh` file, and will review any changes to the file in the future.
@carlosala carlosala self-assigned this Mar 7, 2024
@pepoluan
Copy link
Contributor Author

pepoluan commented Mar 8, 2024

One thing I'm not sure is where to document the zstyle :omz subexecutor knob.

(Setting that knob before starting up OMZ will disable the auto-detection mechanism and 'locks' the subexecutor for the session.)

@pepoluan
Copy link
Contributor Author

Just synced with latest OMZ master. No conflicts.

Also documents the usage of `zstyle ':omz' subexecutor` to explicitly set the subexecutor being used.
@ohmyzsh ohmyzsh bot added the Type: documentation Documentation issue or Pull Request label Mar 13, 2024
README.md Outdated
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have decided to document the subexecutor auto-detection (and its related zstyle-based configuration knob) in OMZ's README.md, especially since there are mentions of other possible zstyle-based configuration in this file.

@pepoluan
Copy link
Contributor Author

1 month since last changes from my part (commit 7ddee97), and so far merging from ohmyzsh:master is no problem, and no conflicts.

So, any blocking issues, @carlosala ?

@carlosala
Copy link
Member

Hi @pepoluan! It's on our backlog, and we want to rethink it before merging. We're shaping a new architecture for omz that will arrive at some point, and we really want to make this part of it.
I'll get back to you as soon as we decide something!
Thanks for your work!

@pepoluan
Copy link
Contributor Author

pepoluan commented May 2, 2024

Hi @pepoluan! It's on our backlog, and we want to rethink it before merging. We're shaping a new architecture for omz that will arrive at some point, and we really want to make this part of it. I'll get back to you as soon as we decide something! Thanks for your work!

Ah okay, fair enough.

In the meantime I will keep updating the branch to sync with master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: core Issue or PR related to core parts of the project Topic: alias Pull Request or issue regarding aliases Type: documentation Documentation issue or Pull Request
Projects
Status: Backlog
Development

Successfully merging this pull request may close these issues.

Provide OMZ-wide option to specify subexecutor such as doas
2 participants