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

plasma-5: 5.16.5 -> 5.17.0 #71232

Closed
wants to merge 6 commits into from
Closed

Conversation

@nyanloutre
Copy link
Member

@nyanloutre nyanloutre commented Oct 16, 2019

Motivation for this change

I am still building and testing, some component might be broken

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • 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 @

@nyanloutre
Copy link
Member Author

@nyanloutre nyanloutre commented Oct 16, 2019

I am currently blocked by the plasma startup script which has been completely rewritten in C++ (was previously made in Bash)

@worldofpeace
Copy link
Member

@worldofpeace worldofpeace commented Oct 16, 2019

I am currently blocked by the plasma startup script which has been completely rewritten in C++ (was previously made in Bash)

Which component? Can you link the current expression and the patch?
We can then examine the C++ code and see what needs to be implemented/re-implemented in the patch for it.

Edit: guessing source is

old patch is

Cc @ttuegel ^

@nyanloutre
Copy link
Member Author

@nyanloutre nyanloutre commented Oct 16, 2019

It's in this patch pkgs/desktops/plasma-5/plasma-workspace/plasma-workspace.patch

And the startup scripts are in plasma-workspace-5.17.0/startkde:
startkde.cmake is now startplasma-x11.cpp
startplasma.cmake is now startplasma.cpp
startplasmacompositor.cmake is now startplasma-wayland.cpp and startplasma-waylandsession.cpp

@ttuegel
Copy link
Member

@ttuegel ttuegel commented Oct 20, 2019

I am currently blocked by the plasma startup script which has been completely rewritten in C++ (was previously made in Bash)

Although we "patch" the startup scripts, ours versions are essentially a rewrite. Over the years, almost every line of the original script has been replaced. Many, many things are broken in upstream's scripts in ways that are not obvious at first. If we want to patch the new C++ startup files, we will need to go through line by line in comparison with the shell scripts.

What a completely useless vanity rewrite. 😠

Unless you can find some new functionality (I looked at the source and I can't): I strongly recommend that, instead of patching upstream's scripts, we delete startplasma and friends in postInstall and replace them with our existing scripts.

@acowley
Copy link
Contributor

@acowley acowley commented Oct 20, 2019

Faster startup due to this rewrite was a headline feature of this release. Not that I’m struggling with the existing startup time, but it is something that I’ve seen users on, eg, Reddit speak about in a positive way.

@worldofpeace
Copy link
Member

@worldofpeace worldofpeace commented Oct 20, 2019

I've actually noticed plasma's startuptime regress since using qt wrappers, not sure how that produces an overhead, but if you look at any of the screenshots of the vm tests it rarely draws the desktop upon completion. Not sure if a C++ startup script would assist in that regard.

@ttuegel, right it's essentially a rewrite. So the C++ files should probably be substituted with rewritten one's during the build, if that's the route chosen.

@peterhoeg
Copy link
Member

@peterhoeg peterhoeg commented Oct 28, 2019

I'm with @ttuegel on this. If the only thing blocking is the start-up, then ideally we (I'm aware that I'm saying "we" without having done any actual work on this) simply release with the old script (assuming it works) and we can then sort out the cpp version later.

@ttuegel
Copy link
Member

@ttuegel ttuegel commented Nov 9, 2019

I've actually noticed plasma's startuptime regress since using qt wrappers, not sure how that produces an overhead, but if you look at any of the screenshots of the vm tests it rarely draws the desktop upon completion.

Right, any kind of wrappers make start-up very slow because the shell is quite slow. We could replace Bash with a more minimal shell.

(The truth is, it may not be possible to support Qt/KDE without an FHS chroot. 😦)

@FRidh FRidh added this to WIP in Staging Nov 12, 2019
nyanloutre added 6 commits Oct 16, 2019
@nyanloutre nyanloutre force-pushed the nyanloutre:plasma_update_5_17_0 branch from c1df5e2 to e260282 Nov 14, 2019
@nyanloutre nyanloutre changed the title plasma-5: 5.16.5 -> 5.17.0 kdeFrameworks: 5.62 -> 5.63 plasma-5: 5.16.5 -> 5.17.0 Nov 14, 2019
@nixos-discourse
Copy link

@nixos-discourse nixos-discourse commented Dec 10, 2019

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/when-is-kde-updated/4882/4

@peterhoeg
Copy link
Member

@peterhoeg peterhoeg commented Dec 10, 2019

FYI, 5.17.4 and frameworks 5.64 are out.

@nyanloutre
Copy link
Member Author

@nyanloutre nyanloutre commented Dec 10, 2019

KDE frameworks 5.64 already got merged in #73304

@worldofpeace worldofpeace added this to the 20.03 milestone Dec 12, 2019
@ttuegel
Copy link
Member

@ttuegel ttuegel commented Jan 26, 2020

@nyanloutre I had a chance to look at this today. I needed to rebase onto master (branch) to fix an unrelated broken build. I'm now getting an error building kwin:

In file included from /nix/store/2pfj5rfcifmwc0db5jkydmhdcgkg5s3f-qtbase-5.12.6-dev/include/QtCore/qobject.h:54,
                 from /nix/store/2pfj5rfcifmwc0db5jkydmhdcgkg5s3f-qtbase-5.12.6-dev/include/QtCore/qiodevice.h:45,
                 from /nix/store/2pfj5rfcifmwc0db5jkydmhdcgkg5s3f-qtbase-5.12.6-dev/include/QtCore/qtextstream.h:43,
                 from /nix/store/2pfj5rfcifmwc0db5jkydmhdcgkg5s3f-qtbase-5.12.6-dev/include/QtCore/qdebug.h:49,
                 from /nix/store/2pfj5rfcifmwc0db5jkydmhdcgkg5s3f-qtbase-5.12.6-dev/include/QtCore/qloggingcategory.h:44,
                 from /nix/store/2pfj5rfcifmwc0db5jkydmhdcgkg5s3f-qtbase-5.12.6-dev/include/QtCore/QLoggingCategory:1,
                 from /build/kwin-5.17.0/build/plugins/qpa/logging.h:6,
                 from /build/kwin-5.17.0/plugins/qpa/eglhelpers.cpp:24:
/nix/store/2pfj5rfcifmwc0db5jkydmhdcgkg5s3f-qtbase-5.12.6-dev/include/QtCore/qmetatype.h:59:2: error: #error qmetatype.h must be included before any header file that defines Bool
   59 | #error qmetatype.h must be included before any header file that defines Bool
      |  ^~~~~

Have you seen this before?

@nyanloutre
Copy link
Member Author

@nyanloutre nyanloutre commented Jan 27, 2020

@ttuegel hello, I am not sure about this one. Do you have the same problem with the last version of plasma ? (5.17.5)

@ttuegel
Copy link
Member

@ttuegel ttuegel commented Jan 27, 2020

Do you have the same problem with the last version of plasma ? (5.17.5)

No, it seems to be fixed in kwin-5.17.5. Thanks!

@ttuegel
Copy link
Member

@ttuegel ttuegel commented Feb 1, 2020

Closing in favor of #79011. I have a newer version with patches for the startup process, but there are a few other unsolved issues.

@ttuegel ttuegel closed this Feb 1, 2020
Staging automation moved this from WIP to Done Feb 1, 2020
@nyanloutre nyanloutre deleted the nyanloutre:plasma_update_5_17_0 branch Sep 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Staging
  
Done
Linked issues

Successfully merging this pull request may close these issues.

None yet

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