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

Follow Debian guidelines for launching editors #157

Merged
merged 2 commits into from
May 7, 2024

Conversation

maxnordlund
Copy link
Contributor

This is how most other tools work when they want the user to edit a file.

@Lockszmith-GH
Copy link
Contributor

Thanks for implementing this, tested and works.

@maxnordlund
Copy link
Contributor Author

maxnordlund commented May 6, 2024

I missed that issue, and VISUAL. So I've included that as well (and squashed the commits while I was at it).

It's not strictly correct though, as it doesn't check if VISUAL or EDITOR points to an executable file. But I figure if someone sets EDITOR they probably know what they are doing 😄

@Lockszmith-GH
Copy link
Contributor

thanks @maxnordlund, obviously, still works - just tested it.

Just a reminder that /usr/bin/editor in debian by default points to nano, so editor instead of nano makes the solution completly in compliance of debain's guidelines. (allowing for system-wide defaults to be changed with update-alternatives)

@Jip-Hop
Copy link
Owner

Jip-Hop commented May 7, 2024

If we want to be really complete then we need to check if the command exists too. I think a get_text_editor method would make sense instead of the current global variable. It could loop over VISUAL and EDITOR and check each time if it is a command using shutil.which and return this string if it is a valid command. If neither VISUAL and EDITOR have returned then return /usr/bin/editor.

This has the added benefit of making this bit redundant:

jailmaker/jlmkr.py

Lines 1509 to 1514 in 4867555

if not shutil.which(TEXT_EDITOR):
eprint(
f"Unable to edit config file: {jail_config_path}.",
f"\nThe {TEXT_EDITOR} text editor is not available",
)
return 1

@maxnordlund maxnordlund changed the title Adhere to EDITOR Follow Debian guidelines for launching editors May 7, 2024
@maxnordlund
Copy link
Contributor Author

OK, so I took the time to do this properly. It now closely follows the Debian guidelines, including both looking up editor before falling back to /usr/bin/editor, and if all else fails falls back to the old hardcoded nano.

When jailmaker wants to ask the user to edit a file, mostly a jail
config, it currently uses `nano`. This is not how a program is supposed
to work according to [Debian's guidelines]. This changes the hardcoded
`nano` to look up the correct editor to use using environmental
variables.

[1]: https://www.debian.org/doc/debian-policy/ch-customized-programs.html#editors-and-pagers
@Jip-Hop Jip-Hop merged commit 487b0cd into Jip-Hop:main May 7, 2024
@maxnordlund maxnordlund deleted the patch-1 branch May 7, 2024 18:12
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.

3 participants