When I look at examples for creating .desktop files, it's recommended to use the full path to the binary. One practical advantage to this is when the --prefix is changed when running `./configure`. For example, if I use `--prefix=$HOME/.local`, Geany will show in my desktop menu, but even though $HOME/.local/bin is already included in my PATH, Geany won't execute because the `Exec` line in the desktop file is simply 'geany'.
I included some other trivial changes in this PR to make more practical use of the new 'geany.desktop.in.in' file. You can view, comment on, or merge this pull request online at:
https://github.com/geany/geany/pull/2728
-- Commit Summary --
* writes absolute path to geany bin in desktop file
-- File Changes --
M Makefile.am (2) M configure.ac (4) R geany.desktop.in.in (6) M geany.pc.in (4) M m4/geany-utils.m4 (12)
-- Patch Links --
https://github.com/geany/geany/pull/2728.patch https://github.com/geany/geany/pull/2728.diff
The idea seems ok to me, desktop menu systems these days are started by systemd not as the user, so user changes to PATH are not available to the desktop, so setting the full path seems sensible.
My Autofools isn't good enough to comment on the implementation.
CI issues need fixing.
When I look at examples for creating .desktop files, it's recommended to use the full path to the binary.
Best source for info is [the spec](https://specifications.freedesktop.org/desktop-entry-spec/desktop-entry-spec...):
The executable program can either be specified with its full path or with the name of the executable only. If no full path is provided the executable is looked up in the $PATH environment variable used by the desktop environment.
@codebrainz neat, so the spec allows full path.
Yeah, it doesn't recommend either way.
I'm not sure what the consequences of hard-coding an absolute path in practice are - particularly in the case of multiple Geany installations - versus doing a `PATH` lookup, so I do not have an opinion, other than that the unrelated changes in this PR aren't super compelling.
Ping @dmaphy, @hyperair, @b4n et al.
versus doing a PATH lookup
The problem is (and as the spec says) its the _desktop_ PATH, not the users PATH, so it will only ever work if Geany is in a standard system PATH location, and putting stuff there usually needs root or sudo.
I have no idea if other stuff (DEs, menu editors, etc.) purposefully use the DE's `PATH` variable in all configurations, or whether it is common for DEs to run under other than the currently logged in user, so not sure, no opinion.
Good point, on my Mint a minor survey shows about 50/50 eg firefox is just the command, chrome is the full path, scite is the command, eclipse is the full path etc
@andy5995 pushed 1 commit.
786a4caa9969e170910694d2cbc24fa603c74e63 specify path to geany.desktop.in in POTFILES.in
Trying to fix the po issue... heading into spaghetti territory with my [last commit](https://github.com/geany/geany/pull/2728/commits/786a4caa9969e170910694d2cbc...) (which doesn't work anyway, and won't without more of a mess I think).
https://travis-ci.org/github/geany/geany/jobs/754726914#L1809
Might be time to abandon this PR and seek out a better approach. I think it would be best if this could be done using the already-existing `geany.desktop.in` file. But I don't know how to do that.
I see this in the main automake file
``` desktop_in_files = geany.desktop desktop_DATA = $(desktop_in_files:.desktop.in=.desktop) @INTLTOOL_DESKTOP_RULE@ ```
But I'm not familiar yet with this process of generating the desktop(out) file (as opposed to using './configure').
This looks like someone trying to do something similar to what I"m trying to accomplish, using the in.in file when INTLTOOL_DESKTOP_RULE is also being used to create an out file... https://stackoverflow.com/questions/12239624/intltool-with-an-autoconf-gener...
@andy5995 pushed 1 commit.
4082f98202a00b7ff89afc231992c575e539b45f use pkg description in about.c and libmain.c as well
@andy5995 pushed 1 commit.
a3fdfe10241b1c1a5d03673a8c142bfda89e07ec Revert "use pkg description in about.c and libmain.c as well"
@b4n requested changes on this pull request.
I am not sure we do want this, but maybe. I don't see much downside, but perhaps when building with `--enable-binreloc` (gives `Exec=//bin/geany` which is… well, both right an wrong), yet I don't know how much sense a .desktop makes for a relocatable installation. So let's say we're happy with it.
However, I don't like the implementation. IMO, we should rather do something like https://github.com/b4n/geany/commit/884309bd6e7969f7b2d01592fa6741cd76b1580b -- and AFAIK it's how other do.
This said, full path is not a popular trend in my Debian stable, which kind of makes me wonder if there is a downside I can't think about: ```shell $ rgrep 'Exec *= */' /usr/share/applications/ | wc -l # absolute paths 27 $ rgrep 'Exec *= *[^/]' /usr/share/applications/ | wc -l # non-absolute paths 233 ```
@@ -11,6 +11,18 @@ AC_DEFUN([GEANY_PREFIX],
if test "x$exec_prefix" = xNONE; then exec_prefix=$prefix fi + + # The $bindir variable is equal to the literal string "${exec_dir}/bin" + # rather than the actual path. pkgconfig (.pc) files can use that ok, but + # other files written out by configure require the literal path. + # + # If the --bindir argument is given to configure, then $bindir will already + # be equal to an absolute path.
Actually no, AFAIK it's valid to pass `--bindir` with placeholders. The only "reliable" way of expanding one of the dir variables is to eval it until it stops changing -- which is both annoying and tricky. IIRC there's a 3rd party macro to do so, but in any case it's not great.
The best solution usually is to perform these replacements at make time.
@@ -9,6 +9,9 @@ AC_CONFIG_MACRO_DIR([m4])
AM_INIT_AUTOMAKE([1.11 -Wall parallel-tests subdir-objects]) AC_CONFIG_HEADERS([config.h])
+PKG_DESC="A fast and lightweight IDE using GTK+"
I think that's kind of over-engineering the thing, and IMO should at least not be part of this change.
+Name: @PACKAGE_NAME@
+Description: @PKG_DESC@
This is unrelated to the proposed change
Thinking about it, distros can use just the command because they install programs in system directories, which are in the default PATH, so the desktop will look there. So it is expected that all of Debian `.desktop`s will only have the command.
Bright packagers like @hyperair will set the `.desktop` to suit their distros rules anyway.
So @b4n what Debian does for software in its repo is not good guidance for what we should generate in a default `.desktop` that may be installed outside system directories.
For user installs outside the system directories the path is needed because it isn't installed in a system directory, so the desktop will not find it in its PATH, so no install outside system directories will work with a `.desktop` otherwise.
So the `.desktop` we generate on a build should have the full path.
So @b4n what Debian does for software in its repo is not good guidance for what we should generate in a default `.desktop` that may be installed outside system directories.
I don't think Debian goes to any length to tamper with a working upstream Desktop file, so that really seem like an upstream thing to me (and I checked a few, and they are the same as I see).
For user installs outside the system directories the path is needed because it isn't installed in a system directory, so the desktop will not find it in its PATH, so no install outside system directories will work with a `.desktop` otherwise.
Well, the *only* case where it really is that simple is the OP's one, i.e. `--prefix=$HOME/.local`. In all other situations the desktop file will not be located automatically by the DE either, so it'd require manual tampering. Yes, it's easier to just have to move/link the Desktop file than modify it, but it sill doesn't work out of the box.
In any case, again, I don't see a real downside to having a full path, and I see the upside (even if not huge), so why not.
Hmmm, yes the autotools default of `$datadir/applications` is pretty much only occasionally right if not a system location.
Just a query, shouldn't it be called `org.geany.Geany.desktop`?
Hmmm, yes the autotools default of `$datadir/applications` is pretty much only occasionally right if not a system location.
Looking at config.log... ``` datadir='${datarootdir}' datarootdir='${prefix}/share' ``` So $datadir will change depending on $prefix.
Just a query, shouldn't it be called `org.geany.Geany.desktop`?
I see that format used in [appdata](https://github.com/geany/geany/issues/2729) files, but I don't see any desktop files with that filename.
The emacs appdata file for example has lines like
``` <component type="desktop-application"> <id>org.gnu.emacs</id> <launchable type="desktop-id">emacs</launchable> ```
and 0 A.D.
``` <component type="desktop-application"> <id>com.play0ad.zeroad</id> <launchable type="desktop-id">0ad.desktop</launchable> ```
So $datadir will change depending on $prefix.
Yep, but desktop files are in fixed locations defined in $XDG_DATA_DIRS so unless thats updated to point to wherever $prefix/share/applications says to install it, the desktop file won't be found, its just lucky your install location is in your $XDG_DATA_DIRS, ~/.local is not in mine for eg. (that was the point @b4n made)
Just a query, shouldn't it be called org.geany.Geany.desktop?
I see that format used in appdata files, but I don't see any desktop files with that filename.
Sorry should have put the reference, the [desktop file spec](https://specifications.freedesktop.org/desktop-entry-spec/desktop-entry-spec...) says "For applications, the part of the name of the desktop file (before the .desktop) should follow the "reverse DNS" convention, e.g. org.example.FooViewer.desktop."
@andy5995 pushed 3 commits.
639e610a819d01215981e27d76ab2d22aee82f4b Revert "specify path to geany.desktop.in in POTFILES.in" 30de359bb0d43e00c02dda8fd59bdadaecb6d379 Revert "writes absolute path to geany bin in desktop file" d16ead846bb7f8e6c0c694c0ae5efc2e713cf1ff use sed to replace Exec line with path
However, I don't like the implementation. IMO, we should rather do something like [b4n@884309b](https://github.com/b4n/geany/commit/884309bd6e7969f7b2d01592fa6741cd76b1580b) -- and AFAIK it's how other do.
I wasn't happy with it either. I've made some changes.
@andy5995 pushed 1 commit.
e2d328c0e0644ac5a8da33e0e4182849dbc7da74 append '%F' to Exec line
@codebrainz commented on this pull request.
@@ -37,6 +37,7 @@ uninstall-local:
# manually install some files under another name install-data-local: + sed -i '/Exec=/c\Exec=@bindir@/geany %F' @top_builddir@/geany.desktop
Can't you just do this:
```diff - Exec=geany %F + Exec=@bindir@/geany %F ```
In [the `geany.desktop.in` file](https://github.com/geany/geany/blob/d9f8cdbad58d09f0c18ca8acccb49209263018f0...) or similar?
I'm pretty sure if you added it to `AC_CONFIG_FILES` it would get the substitutions. But maybe I'm missing something of Geany's Autotools.
@b4n commented on this pull request.
@@ -37,6 +37,7 @@ uninstall-local:
# manually install some files under another name install-data-local: + sed -i '/Exec=/c\Exec=@bindir@/geany %F' @top_builddir@/geany.desktop
@codebrainz It's not that simple, because all the `*dir` variables usually expand to placeholders like `${prefix}/bin` and such. So you need to expand them first, and as mentioned in another comment, it's not super trivial. Plus, in some situations they can be overridden up until make calls, so it's better to apply them there.
@b4n requested changes on this pull request.
This has a few issues IMO: * The way you modify the file *changes the whole line*, meaning that if that rule and the source file are not in sync, it's super hard to diagnose weird content in the installed file (as it ignores the content of the source file). * I don't like modifying a file at install time. Not only this means you get another set of files before and after installation, this also means that for a lot of people it'll modify build files *as root* (if they install as root), which is awfully annoying (you get root-owned files you can't modify in a user directory). * The [documentation](https://www.gnu.org/software/automake/manual/automake.html#Extending-Install...) doesn't actually say whether or not `install-data-local` is performed before or after the Automake rules, so it's not guaranteed modifying a file will install it. In practice it's probably the case if it currently works (and I guess the implementation is straightforward and guarantees it), but it's undocumented.
I'll abuse of my dictatorship here and say that if we want this we'll use my set of changes (https://github.com/b4n/geany/commit/884309bd6e7969f7b2d01592fa6741cd76b1580b), unless of course anybody has specific concerns about it.
Sorry should have put the reference, the [desktop file spec](https://specifications.freedesktop.org/desktop-entry-spec/desktop-entry-spec...) says "For applications, the part of the name of the desktop file (before the .desktop) should follow the "reverse DNS" convention, e.g. org.example.FooViewer.desktop."
@elextr this is orthogonal to the issue at hand, but: * Yes, I think it's now the preferred naming scheme; * However, I don't know if it actually has any advantage (I though at a time it helped with startup notification, but tests I made a while back suggests it does not); * I know downsides to changing a .desktop file name (dead links, duplicated entries if people copied over the file, and whatnot).
So… yeah if we'd start over, I'd use the reverse-DNS scheme, but I don't think we should change the name we use unless we really know *why*.
@codebrainz commented on this pull request.
@@ -37,6 +37,7 @@ uninstall-local:
# manually install some files under another name install-data-local: + sed -i '/Exec=/c\Exec=@bindir@/geany %F' @top_builddir@/geany.desktop
Ah ok. I was thinking it like the pkg-config file where do this, but I guess there we want them unexpanded because pkg-config takes care of it.
@elextr this is orthogonal to the issue at hand, but:
But if we are stuffing about with the file it might have been an appropriate time to do it.
Yes, I think it's now the preferred naming scheme; However, I don't know if it actually has any advantage (I though at a time it helped with startup notification, but tests I made a while back suggests it does not);
Possibly its as simple as to avoid name clashes, but thats unlikely to be a problem for us. Also the name [is used on DBUS](https://specifications.freedesktop.org/desktop-entry-spec/desktop-entry-spec...) so it needs to be DBUS format. Of course Geany doesn't use that, so it doesn't matter to us.
I know downsides to changing a .desktop file name (dead links, duplicated entries if people copied over the file, and whatnot).
Well an install with a new filename could always remove any old `*geany.desktop` filenames, currently we claim the whole of that namespace so its _somebody else's problem_ if they invaded our namespace `</channelling G*>` :grin:.
So… yeah if we'd start over, I'd use the reverse-DNS scheme, but I don't think we should change the name we use unless we really know why.
But but its _new_ and _techo_ ... oh ok fair enough :grin:.
This has a few issues IMO:
* The way you modify the file _changes the whole line_, meaning that if that rule and the source file are not in sync, it's super hard to diagnose weird content in the installed file (as it ignores the content of the source file). * I don't like modifying a file at install time. Not only this means you get another set of files before and after installation, this also means that for a lot of people it'll modify build files _as root_ (if they install as root), which is awfully annoying (you get root-owned files you can't modify in a user directory). * The [documentation](https://www.gnu.org/software/automake/manual/automake.html#Extending-Installation) doesn't actually say whether or not `install-data-local` is performed before or after the Automake rules, so it's not guaranteed modifying a file will install it. In practice it's probably the case if it currently works (and I guess the implementation is straightforward and guarantees it), but it's undocumented.
Hmm.. well points 2 and 3 are easily solved by moving the change so that it happens when 'make' is run without 'install'. I definitely agree that we can't have root modifying the file during install after it's been created by someone else when just 'make' is run. Though
it'll modify build files as root (if they install as root), which is awfully annoying (you get root-owned files you can't modify in a user directory).
I don't think the file would ever be owned by root if created by someone else, and it's just changed during 'make install'. But still, it would be better to have the modification done during 'make' w/o install.
I only kind of understood your first point.
* The way you modify the file _changes the whole line_, meaning that if that rule and the source file are not in sync, it's super hard to diagnose weird content in the installed file (as it ignores the content of the source file)
If I understood it right, that's easy enough to fix by changing the Exec line to something like `Exec=DEST_BIN_PATH` and then have sed look for that exact line and do a replacement, and throw an error if the match isn't found at all.
I'll abuse of my dictatorship here and say that if we want this we'll use my set of changes ([b4n@884309b](https://github.com/b4n/geany/commit/884309bd6e7969f7b2d01592fa6741cd76b1580b)), unless of course anybody has specific concerns about it.
That's fine by me, whatever works and you find satisfactory. :) Thank you for explaining your reasoning.
Closed #2728.
github-comments@lists.geany.org