-
-
Notifications
You must be signed in to change notification settings - Fork 14.6k
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
Setup hook to make Exec= paths absolute in desktop files #68035
Conversation
this setup hook rewrites Exec= to an absolute path in desktop entries, like what patchShebangs does to scripts.
fi; | ||
done | ||
if [[ -z $success ]]; then | ||
echo "Warning: $desktopFile has Exec=$execname which is not found in $dirs or is not executable."; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer that this be an error instead of a warning if the $execname
is not found. The warning is too easy to miss.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
GLib has a keyfile parser but it is not very Pythonic: #!/usr/bin/env nix-shell
#!nix-shell -p python3.pkgs.pygobject3 -p glib -i python3
import gi
gi.require_version('GLib', '2.0')
import argparse
import sys
from gi.repository import GLib
from pathlib import Path
PATH_KEYS = [
GLib.KEY_FILE_DESKTOP_KEY_EXEC,
GLib.KEY_FILE_DESKTOP_KEY_TRY_EXEC,
]
def absolutize(value, bindir):
executable = bindir / Path(value).name
if executable.is_file():
return executable
else:
raise FileNotFoundError(f'{executable} does not exist')
if __name__ == '__main__':
parser = argparse.ArgumentParser(description='Absolutize desktop files')
parser.add_argument('prefix', help='Path where to base applicationsdir and bindir on')
args = parser.parse_args()
applicationsdir = Path(args.prefix, 'share', 'applications')
bindir = Path(args.prefix, 'bin')
for desktop_path in applicationsdir.iterdir():
print(f'Patching {desktop_path}', file=sys.stderr)
if desktop_path.suffix == '.desktop':
desktop = GLib.KeyFile()
desktop.load_from_file(desktop_path.as_posix(), GLib.KeyFileFlags.KEEP_TRANSLATIONS | GLib.KeyFileFlags.KEEP_COMMENTS)
groups, _len = desktop.get_groups()
for group in groups:
for key in PATH_KEYS:
try:
value = desktop.get_string(group, key)
if value:
# TODO: parse the Exec lines according to https://specifications.freedesktop.org/desktop-entry-spec/latest/ar01s07.html
print(f'\tReplacing [{group}]{key}={value}', file=sys.stderr)
executable, rest = value.split(' ', 1)
if '"' in executable:
print(f'\t\tSkipping executable containing quotes', file=sys.stderr)
else:
new_value = f'{absolutize(executable, bindir)} {rest}'
desktop.set_string(group, key, new_value)
except GLib.Error as e:
if not e.code == GLib.KeyFileError.KEY_NOT_FOUND:
raise e
desktop.save_to_file(desktop_path.as_posix()) |
continue;; | ||
*) | ||
# we look in $out/bin and buildInputs | ||
dirs="${!outputBin}/bin:$HOST_PATH"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
About writing the setup hook in python: I have the secret hope that if this setup hook proves useful, it may become part of stdenv, like patchShebangs. If this is the case, writing it in bash is a plus. |
Could we get a more specific name than Could we also document the hook? |
Would |
|
afa2b52
to
82acf6c
Compare
Hello, I'm a bot and I thank you in the name of the community for your contributions. Nixpkgs is a busy repository, and unfortunately sometimes PRs get left behind for too long. Nevertheless, we'd like to help committers reach the PRs that are still important. This PR has had no activity for 180 days, and so I marked it as stale, but you can rest assured it will never be closed by a non-human. If this is still important to you and you'd like to remove the stale label, we ask that you leave a comment. Your comment can be as simple as "still important to me". But there's a bit more you can do: If you received an approval by an unprivileged maintainer and you are just waiting for a merge, you can @ mention someone with merge permissions and ask them to help. You might be able to find someone relevant by using Git blame on the relevant files, or via GitHub's web interface. You can see if someone's a member of the nixpkgs-committers team, by hovering with the mouse over their username on the web interface, or by searching them directly on the list. If your PR wasn't reviewed at all, it might help to find someone who's perhaps a user of the package or module you are changing, or alternatively, ask once more for a review by the maintainer of the package/module this is about. If you don't know any, you can use Git blame on the relevant files, or GitHub's web interface to find someone who touched the relevant files in the past. If your PR has had reviews and nevertheless got stale, make sure you've responded to all of the reviewer's requests / questions. Usually when PR authors show responsibility and dedication, reviewers (privileged or not) show dedication as well. If you've pushed a change, it's possible the reviewer wasn't notified about your push via email, so you can always officially request them for a review, or just @ mention them and say you've addressed their comments. Lastly, you can always ask for help at our Discourse Forum, or more specifically, at this thread or at #nixos' IRC channel. |
I marked this as stale due to inactivity. → More info |
} | ||
|
||
patchDesktopFileExecIn () { | ||
echo "fixing Exec= paths in desktop files of $1"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
echo "fixing Exec= paths in desktop files of $1"; | |
echo "fixing Exec= paths in desktop files of $1" |
bash does not require end of line ; except when having multiple commands on one line.
# we look in $out/bin and buildInputs | ||
dirs="${!outputBin}/bin:$HOST_PATH"; | ||
success=; | ||
IFS=: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a proper reset?
patchDesktopFileExecHook = makeSetupHook {} | ||
../build-support/setup-hooks/patch-desktop-file-exec.sh; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
patchDesktopFileExecHook = makeSetupHook {} | |
../build-support/setup-hooks/patch-desktop-file-exec.sh; | |
patchDesktopFileExecHook = makeSetupHook { } ../build-support/setup-hooks/patch-desktop-file-exec.sh; |
Thanks for the reviews, but before I delve into stylistic changes, it should be established that this is a good idea at all... |
Just found this when encountering #141873 . This hook is pretty useful but unfortunately still blocking after 2yrs. Personally, I think absolute paths in I think we should patch them to absolute path by default, and provide a flag |
If I remember correctly, the point is that some programs like the menu editor of some DE copy .desktop files in, say, ~/.something. If they contain an absolute path, the gc can remove the corresponding paths and break the custom menu entries. So whether absolute paths in desktop files is a good idea does not depend on what desktop file, but rather on what software uses it. So making the absolutification of paths optional does not seem like a good solution to me. A few options we have:
What do you think ? |
We all mentioned there are some programs relying on relative paths since they need copy desktop files. But it's worth mentioning that there are also some programs relying on absolute paths. KWin expects absolute |
The binary path thing is a bit messy overall and has caused problems before with our habit of using wrapper scripts for basically all GUI applications, which results in the actual binary path being different from the path in the desktop file and failure to associate a desktop file with a given process. This is currently worked around in kwin with a patch that tries to find the topmost wrapper by cutting off parts of the path until it contains no more |
Motivation for this change
Using sed to replace a path in a desktop file with an absolute one is a common idiom in nixpkgs: https://search.nix.gsc.io/?q=(sed%7Csubstitute).*desktop&i=nope&files=&repos= has about 150 matches.
This setup hook automates this in a similar fashion as patchShebangs. A selection of derivations are converted to using this setup hook both to test and illustrate its usage.
(the deepin changes were tested the last revision of nixpkgs where it builds)
The setup hook is designed to be able to handle several Exec= lines in a single desktop file, and handle both existing and inexsting (think
/usr/bin
), absolute and relative paths. It will bail out if the Exec= line is quoted because I am unable to implement this in bash, and I found no perl or python library which can both read and write properly quoted argv arrays in desktop files.Things done
sandbox
innix.conf
on non-NixOS)nix-shell -p nix-review --run "nix-review wip"
nix path-info -S
before and after)./result/bin/
)