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

Fix linux desktop files validation #3817

Closed
wants to merge 1 commit into from
Closed

Fix linux desktop files validation #3817

wants to merge 1 commit into from

Conversation

voyageur
Copy link
Contributor

Follow quoting rules from:
https://specifications.freedesktop.org/desktop-entry-spec/desktop-entry-spec-latest.html#exec-variables Also use /bin/sh for scrpy-console

This fixes #3633 #3633
Downstream Gentoo bug: https://bugs.gentoo.org/888133

It adds a few backslashes but makes desktop-file-validate happy

@rom1v
Copy link
Collaborator

rom1v commented Mar 14, 2023

Thank you for the PR.

I'm ok to change the quotes so that it pass validation by desktop-file-validate, but the command for scrcpy-console.desktop use the bash-specific version of the built-in read function (and since it uses bash it also pass --norc and --noprofile), so we cannot just change bashtosh`.

I suggest:

diff --git a/app/data/scrcpy-console.desktop b/app/data/scrcpy-console.desktop
index 47a63ec99..f99217827 100644
--- a/app/data/scrcpy-console.desktop
+++ b/app/data/scrcpy-console.desktop
@@ -5,7 +5,7 @@ Comment=Display and control your Android device
 # For some users, the PATH or ADB environment variables are set from the shell
 # startup file, like .bashrc or .zshrc… Run an interactive shell to get
 # environment correctly initialized.
-Exec=/bin/bash --norc --noprofile -i -c '"$SHELL" -i -c scrcpy || read -p "Press any key to quit..."'
+Exec=/bin/bash --norc --noprofile -i -c "\"\\$SHELL\" -i -c scrcpy || read -p 'Press any key to quit...'"
 Icon=scrcpy
 Terminal=true
 Type=Application
diff --git a/app/data/scrcpy.desktop b/app/data/scrcpy.desktop
index 082b75e0f..1be86a2ba 100644
--- a/app/data/scrcpy.desktop
+++ b/app/data/scrcpy.desktop
@@ -5,7 +5,7 @@ Comment=Display and control your Android device
 # For some users, the PATH or ADB environment variables are set from the shell
 # startup file, like .bashrc or .zshrc… Run an interactive shell to get
 # environment correctly initialized.
-Exec=/bin/sh -c '"$SHELL" -i -c scrcpy'
+Exec=/bin/sh -c "\"\\$SHELL\" -i -c scrcpy"
 Icon=scrcpy
 Terminal=false
 Type=Application

What do you think?

@voyageur
Copy link
Contributor Author

Sorry for the noise, I now remember I switched back my /bin/sh to bash (from dash)... So of course using /bin/sh worked

Let me update the PR restoring bash in scrcpy-console

rom1v pushed a commit that referenced this pull request Mar 14, 2023
@rom1v
Copy link
Collaborator

rom1v commented Mar 14, 2023

Thank you 👍

Rebased on dev and merged: d2b7315

@rom1v rom1v closed this Mar 14, 2023
@voyageur voyageur deleted the fix_desktop_file branch March 14, 2023 22:25
@ratijas
Copy link

ratijas commented Jun 28, 2023

Hi,

I noticed this MR because after recent update I saw new warnings in terminal:

kf.config.core: "KConfigIni: In file /usr/share/applications/scrcpy-console.desktop, line 8: " "Invalid escape sequence \"\\\"\"."
kf.config.core: "KConfigIni: In file /usr/share/applications/scrcpy-console.desktop, line 8: " "Invalid escape sequence \"\\\"\"."
kf.config.core: "KConfigIni: In file /usr/share/applications/scrcpy.desktop, line 8: " "Invalid escape sequence \"\\\"\"."
kf.config.core: "KConfigIni: In file /usr/share/applications/scrcpy.desktop, line 8: " "Invalid escape sequence \"\\\"\"."

On a closer inspection, those lines might be valid after all, and we have a bug in KConfig parser instead.

Nevertheless, these lines look a bit over-engineered, but also inconsistent at the same time. Why hardcoding /bin/sh in one .desktop file, /bin/bash in another, and then asking them to execute whatever the \"\\$SHELL\" is pointing to.

Besides, the whole comment indicates that it is a workaround broken user setup in the first place:

# For some users, the PATH or ADB environment variables are set from the shell
# startup file, like .bashrc or .zshrc… Run an interactive shell to get
# environment correctly initialized.

If user needs their environment variables be accessible from apps, they need to put it into appropriate .profile config. Expecting each and every app to "fix" it for them in gazzilion different creative ways isn't going to fly.

@rom1v
Copy link
Collaborator

rom1v commented Jun 28, 2023

Why hardcoding /bin/sh in one .desktop file, /bin/bash in another

Because in one case (scrcpy.desktop) we just execute scrcpy.
But in the other case, we execute scrcpy, and only if it fails we want to keep the terminal open (calling read -p 'Press Enter to quit...'). But this last part depends on the shell used, so we must force bash.

If user needs their environment variables be accessible from apps, they need to put it into appropriate .profile config.

That's true, but in practice, scrcpy is a command-line app, so intended to be started from a terminal.

The point of .desktop in that case is to open a terminal with their default configuration (e.g. .bashrc), so that the behavior is the same as when they open a terminal then type scrcpy.

I agree that this is not pretty. In a sense, it seems that the .desktop files are a workaround to use scrcpy "as if" it was not a command line app (but it is).

@ratijas
Copy link

ratijas commented Jun 28, 2023

Meanwhile I sent a "readability patch" to KConfig and filed a bug report about missing specialized Exec= parsing algorithm in KService/KDesktopFile.

@rom1v
Copy link
Collaborator

rom1v commented Jun 28, 2023

Just to be sure, the current version of scrcpy causes the bug in KConfig, but the previous one was working fine?

@ratijas
Copy link

ratijas commented Jun 28, 2023

Sure. The old variant didn't contain any escape sequences after all.

rom1v added a commit that referenced this pull request Jun 29, 2023
Add an option to make scrcpy pause just before it returns with an error
(a non-zero exit code). This prevents any host terminal to close so that
error messages can be read.

Refs #3817 <#3817>
Refs #3822 <#3822>
rom1v added a commit that referenced this pull request Jun 29, 2023
The terminal opened by scrcpy-console (.bat or .desktop) must not close
if scrcpy terminates with an error, so that error messages can be read.

Refs #3817 <#3817>
Refs #3822 <#3822>
@rom1v
Copy link
Collaborator

rom1v commented Jun 29, 2023

@ratijas Please review #4130, it should not fix your problem but at least is simplifies the -console versions.

rom1v added a commit that referenced this pull request Jun 29, 2023
Add an option to make scrcpy pause just before it returns with an error
(a non-zero exit code). In some cases, this prevents the terminal window
from automatically closing, so that error messages can be read.

Refs #3817 <#3817>
Refs #3822 <#3822>
rom1v added a commit that referenced this pull request Jun 29, 2023
The terminal opened by scrcpy-console (.bat or .desktop) must not close
if scrcpy terminates with an error, so that error messages can be read.

Refs #3817 <#3817>
Refs #3822 <#3822>
rom1v added a commit that referenced this pull request Jun 30, 2023
Add an option to make scrcpy pause on exit.

Three behaviors are possible:
 - always pause on exit:
    --pause-on-exit
    --pause-on-exit=true
 - never pause on exit:
    (no option)
    --pause-on-exit=false
 - pause when scrcpy returns with an error (a non-zero exit code):
    --pause-on-exit=if-error

This is useful to prevent the terminal window from automatically
closing, so that error messages can be read.

Refs #3817 <#3817>
Refs #3822 <#3822>
PR #4130 <#4130>
rom1v added a commit that referenced this pull request Jun 30, 2023
The terminal opened by scrcpy-console (.bat or .desktop) must not close
if scrcpy terminates with an error, so that error messages can be read.

Refs #3817 <#3817>
Refs #3822 <#3822>
PR #4130 <#4130>
rom1v added a commit that referenced this pull request Oct 11, 2023
Add an option to make scrcpy pause on exit.

Three behaviors are possible:
 - always pause on exit:
    --pause-on-exit
    --pause-on-exit=true
 - never pause on exit:
    (no option)
    --pause-on-exit=false
 - pause when scrcpy returns with an error (a non-zero exit code):
    --pause-on-exit=if-error

This is useful to prevent the terminal window from automatically
closing, so that error messages can be read.

Refs #3817 <#3817>
Refs #3822 <#3822>
PR #4130 <#4130>
rom1v added a commit that referenced this pull request Oct 11, 2023
The terminal opened by scrcpy-console (.bat or .desktop) must not close
if scrcpy terminates with an error, so that error messages can be read.

Refs #3817 <#3817>
Refs #3822 <#3822>
PR #4130 <#4130>
@rom1v rom1v mentioned this pull request Nov 21, 2023
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.

desktop-file-validate fails on provided .desktop files
3 participants