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

plasma5.plasma-workspace: fix patch #74830

Merged
merged 1 commit into from Dec 8, 2019
Merged

plasma5.plasma-workspace: fix patch #74830

merged 1 commit into from Dec 8, 2019

Conversation

@Kiwi
Copy link
Contributor

Kiwi commented Dec 2, 2019

At some point a patch accidentally removed

done
break

from an if then/while loop in startkde.

Which still breaks, but not the way we want...

plasma-workspace-5.16.5/bin/startkde: line 403: syntax error near unexpected token `fi'
plasma-workspace-5.16.5/bin/startkde: line 403: `        fi'
Motivation for this change

I've been having more problems with KDE lately than normal and while I was going through my journalctl I came across the error message. I'm not sure if it's related to my logout problems or not but... hopefully.

unpackPhase had this source (startkde.cmake)

if test x"$wait_drkonqi"x = x"true"x ; then
    # wait for remaining drkonqi instances with timeout (in seconds)
    wait_drkonqi_timeout=`kreadconfig5 --file startkderc --group WaitForDrKonqi --key Timeout --default 900`
    wait_drkonqi_counter=0
    while qdbus | grep "^[^w]*org.kde.drkonqi" > /dev/null ; do
        sleep 5
        wait_drkonqi_counter=$((wait_drkonqi_counter+5))
        if test "$wait_drkonqi_counter" -ge "$wait_drkonqi_timeout" ; then
            # ask remaining drkonqis to die in a graceful way
            qdbus | grep 'org.kde.drkonqi-' | while read address ; do
                qdbus "$address" "/MainApplication" "quit"
            done
            break
        fi
    done
fi

After the patchPhase

if [ x"$wait_drkonqi"x = x"true"x ]; then
    # wait for remaining drkonqi instances with timeout (in seconds)
    wait_drkonqi_timeout=$(@NIXPKGS_KREADCONFIG5@ --file startkderc --group WaitForDrKonqi --key Timeout --default 900)
    wait_drkonqi_counter=0
    while @NIXPKGS_QDBUS@ | @NIXPKGS_GREP@ -q "^[^w]*org.kde.drkonqi" ; do
        sleep 5
        wait_drkonqi_counter=$((wait_drkonqi_counter+5))
        if [ "$wait_drkonqi_counter" -ge "$wait_drkonqi_timeout" ]; then
            # ask remaining drkonqis to die in a graceful way
            @NIXPKGS_QDBUS@ | @NIXPKGS_GREP@ 'org.kde.drkonqi-' | while read address ; do
                @NIXPKGS_QDBUS@ "$address" "/MainApplication" "quit"
        fi
    done
fi

What ends up in bin/startkde

if [ x"$wait_drkonqi"x = x"true"x ]; then
    # wait for remaining drkonqi instances with timeout (in seconds)
    wait_drkonqi_timeout=$(/nix/store/s9a9fv0nkq1zaqxcs6p19fg4d6p6nrv2-kconfig-5.62.0-bin/bin/kreadconfig5 --file startkderc --group WaitForDrKonqi --key Timeout --default 900)
    wait_drkonqi_counter=0
    while /nix/store/8cimdfy053rw5lgsm570c4m0rffb4g4g-qttools-5.12.5-bin/bin/qdbus | /nix/store/20b535jb98hy7k4z8vkrlkjma212a3l5-gnugrep-3.3/bin/grep -q "^[^w]*org.kde.drkonqi" ; do
        sleep 5
        wait_drkonqi_counter=$((wait_drkonqi_counter+5))
        if [ "$wait_drkonqi_counter" -ge "$wait_drkonqi_timeout" ]; then
            # ask remaining drkonqis to die in a graceful way
            /nix/store/8cimdfy053rw5lgsm570c4m0rffb4g4g-qttools-5.12.5-bin/bin/qdbus | /nix/store/20b535jb98hy7k4z8vkrlkjma212a3l5-gnugrep-3.3/bin/grep 'org.kde.drkonqi-' | while read address ; do
                /nix/store/8cimdfy053rw5lgsm570c4m0rffb4g4g-qttools-5.12.5-bin/bin/qdbus "$address" "/MainApplication" "quit"
        fi
    done
fi

What shellcheck had to say about that (in red, really angry!)

In /run/current-system/sw/bin/startkde line 399:
        if [ "$wait_drkonqi_counter" -ge "$wait_drkonqi_timeout" ]; then
                                                                    ^-- SC1009: The mentioned syntax error was in this then clause.


In /run/current-system/sw/bin/startkde line 401:
            /nix/store/8cimdfy053rw5lgsm570c4m0rffb4g4g-qttools-5.12.5-bin/bin/qdbus | /nix/store/20b535jb98hy7k4z8vkrlkjma212a3l5-gnugrep-3.3/bin/grep 'org.kde.drkonqi-' | while read address ; do
                                                                                                                                                                             ^-- SC1073: Couldn't parse this while loop. Fix to allow more checks.
                                                                                                                                                                                                  ^-- SC1061: Couldn't find 'done' for this 'do'.


In /run/current-system/sw/bin/startkde line 403:
        fi
        ^-- SC1062: Expected 'done' matching previously mentioned 'do'.
          ^-- SC1072: Unexpected keyword/token. Fix any mentioned problems and try again.

For more information:
  https://www.shellcheck.net/wiki/SC1061 -- Couldn't find 'done' for this 'do'.
  https://www.shellcheck.net/wiki/SC1062 -- Expected 'done' matching previous...
  https://www.shellcheck.net/wiki/SC1072 -- Unexpected keyword/token. Fix any...

And now, what you've all been waiting for....!

What it looks like with this patch

if [ x"$wait_drkonqi"x = x"true"x ]; then
    # wait for remaining drkonqi instances with timeout (in seconds)
    wait_drkonqi_timeout=$(/nix/store/8h096wmzmxpfz25bxsxm3zyrpaf1iw90-kconfig-5.62.0-bin/bin/kreadconfig5 --file startkderc --group WaitForDrKonqi --key Timeout --default 900)
    wait_drkonqi_counter=0
    while /nix/store/bd56x7j9cas7pg37cm2b9gvzv16qbd3h-qttools-5.12.6-bin/bin/qdbus | /nix/store/20b535jb98hy7k4z8vkrlkjma212a3l5-gnugrep-3.3/bin/grep -q "^[^w]*org.kde.drkonqi" ; do
        sleep 5
        wait_drkonqi_counter=$((wait_drkonqi_counter+5))
        if [ "$wait_drkonqi_counter" -ge "$wait_drkonqi_timeout" ]; then
            # ask remaining drkonqis to die in a graceful way
            /nix/store/bd56x7j9cas7pg37cm2b9gvzv16qbd3h-qttools-5.12.6-bin/bin/qdbus | /nix/store/20b535jb98hy7k4z8vkrlkjma212a3l5-gnugrep-3.3/bin/grep 'org.kde.drkonqi-' | while read address ; do
                /nix/store/bd56x7j9cas7pg37cm2b9gvzv16qbd3h-qttools-5.12.6-bin/bin/qdbus "$address" "/MainApplication" "quit"
            done
            break
        fi
    done
fi

Which makes shellcheck a bit less angry about that and start mentioning the rest of the file should use globbing and that read without -r mangles backslashes. Whatever that means.

Things done

I spent way too long (hours, if not days) trying to figure out how all of this worked... and all just so I could do what ultimately amounts to nothing more than adding two lines to a bash script. Well, not removing two lines from a cmake file. :)

~/projects/nixpkgs tracks NixOS/nixpkgs master, and pr-nixpkgs has this PR... (after unpackPhase and patchPhase)

[nix-shell:~/projects/nixpkgs]$ diff -ru plasma-workspace-5.16.5/ ~/projects/github/pr-nixpkgs/plasma-workspace-5.16.5/
diff -ru plasma-workspace-5.16.5/startkde/startkde.cmake /home/kiwi/projects/github/pr-nixpkgs/plasma-workspace-5.16.5/startkde/startkde.cmake
--- plasma-workspace-5.16.5/startkde/startkde.cmake	2019-12-02 10:33:10.319036592 +0000
+++ /home/kiwi/projects/github/pr-nixpkgs/plasma-workspace-5.16.5/startkde/startkde.cmake	2019-12-02 08:37:56.851321636 +0000
@@ -400,6 +400,8 @@
             # ask remaining drkonqis to die in a graceful way
             @NIXPKGS_QDBUS@ | @NIXPKGS_GREP@ 'org.kde.drkonqi-' | while read address ; do
                 @NIXPKGS_QDBUS@ "$address" "/MainApplication" "quit"
+            done
+            break
         fi
     done
 fi

I don't know that this change would matter; but plasma5 tests still passed.

  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nix-review --run "nix-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
Notify maintainers

cc @

At some point a patch accidentally removed

```
done
break
```
from an if then/while loop in `startkde`.

Which still breaks, but not the way we want...

plasma-workspace-5.16.5/bin/startkde: line 403: syntax error near unexpected token `fi'
plasma-workspace-5.16.5/bin/startkde: line 403: `        fi'
@ttuegel
ttuegel approved these changes Dec 2, 2019
Copy link
Member

ttuegel left a comment

Wow, well spotted! I think the patch must have gotten mangled during an earlier version bump. Thanks for fixing this! 😃

@worldofpeace

This comment has been minimized.

Copy link
Member

worldofpeace commented Dec 2, 2019

Might be worthwhile having an Updating Plasma5 docs in nixpkgs.
Typically with these patches you need to be careful on regenerating them/verfiying them and hopefully everyone does a similar procedure.

@worldofpeace worldofpeace requested a review from dtzWill Dec 2, 2019
@ttuegel ttuegel merged commit f35a715 into NixOS:master Dec 8, 2019
16 checks passed
16 checks passed
plasma5.plasma-workspace on x86_64-darwin No attempt
Details
plasma5.plasma-workspace on x86_64-linux Timed out, unknown build status
Details
Evaluation Performance Report Evaluator Performance Report
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-maintainers matching changed paths to changed attrs...
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A darwin-tested
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release-combined.nix -A tested
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A manual
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A options
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A manual
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A tarball
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A unstable
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
plasma5.plasma-workspace on aarch64-linux Success
Details
@Kiwi

This comment has been minimized.

Copy link
Contributor Author

Kiwi commented Dec 10, 2019

Might be worthwhile having an Updating Plasma5 docs in nixpkgs.
Typically with these patches you need to be careful on regenerating them/verfiying them and hopefully everyone does a similar procedure.

There was a comment in (at least) one of the files that could definitely have been more helpful. I'm not sure if it's entirely inaccurate but it seemed out of date at best and in my case outright misleading. :( The lack of "ensured relevant documentation was up to date" was intentional... I could have probably done a bit more there, but eh, handwaves reasons. >.>

@Kiwi Kiwi deleted the Kiwi:startkde-broken branch Jan 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.