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

catppuccin-kde: 0.2.4 -> 0.2.6 #296564

Merged
merged 2 commits into from
Mar 20, 2024

Conversation

GiggleSquid
Copy link
Contributor

@GiggleSquid GiggleSquid commented Mar 17, 2024

Description of changes

Update catppuccin-kde to 0.2.6 and provided new install script patch.
This release has everything ported to KDE Plasma 6.

See: https://github.com/catppuccin/kde/releases/tag/v0.2.6

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.05 Release Notes (or backporting 23.05 and 23.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@GiggleSquid GiggleSquid marked this pull request as ready for review March 17, 2024 03:42
@ofborg ofborg bot added 11.by: package-maintainer This PR was created by the maintainer of the package it changes 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 10.rebuild-linux: 1 labels Mar 17, 2024
Copy link
Contributor

@pluiedev pluiedev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm pretty sure there's another PR that also does this, but instead of patching it uses a much more radical approach of stubbing out all the tools the installer currently requires but don't actually make sense in a Nix build environment (e.g. wget). I'm hesitant to mark this as an exact duplicate though as there are merits to either approach.

Found it: #294507, though it seems to be still based on 0.2.4

pkgs/data/themes/catppuccin-kde/default.nix Outdated Show resolved Hide resolved
patchShebangs .
stdenvNoCC.mkDerivation
rec {
pname = "kde";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TBH this should be rectified to be catppuccin-kde like all the other Catppuccin themes — I know it's been here for a while but now's the time to do it, I think

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was also considering this. I'll have a look at how #294507 handled the script and work on a version with the "master" catppuccin theme. If anyone has any input on a potential preferred method, I think it would be a helpful discussion

@GiggleSquid
Copy link
Contributor Author

GiggleSquid commented Mar 17, 2024

As it stands at the moment for either PR (This and #294507), it might be better to wait on the outcome of catppuccin/kde#56

Both PRs are available for users to use and build themselves temporarily if necessary.

We can end up with a really clean solution in the "master" catpuccin theme depending on the resolution of the upstream issue above

@pluiedev
Copy link
Contributor

pluiedev commented Mar 17, 2024

As it stands at the moment for either PR (This and #294507), it might be better to wait on the outcome of catppuccin/kde#56

Both PRs are available for users to use and build themselves temporarily if necessary.

We can end up with a really clean solution in the "master" catpuccin theme depending on the resolution of the upstream issue above

Yeah. Best case scenario, catppuccin/kde#56 gets fixed, no patches or stubs necessary, everyone happy with a very clean deriv.

However, it looks like the upstream maintainer is taking a while to get to it, and honestly I think I'd rather let this PR go forward as a stop-gap measure:

  • If Non-interactive, quiet and offline modes for the installer script catppuccin/kde#56 is fixed without any substantial changes, then we could just update the source and remove the patch without any version changes as an internal refactor, providing the end results are the same;
  • This PR builds onto the existing deriv while catppuccin-kde: Do not patch install script, and add modified Lightly theme #294507 pretty much rewrote half of it with some... dubious choices. (I'm not exactly happy with the fact that PR changes the deriv to only output one specific combination of flavor/accent/decoration style, for example) The approach taken there will take more discussion to hammer out, which means even more delays to the end users;
  • Finally, this PR seems to have more momentum due to its simplicity. The creator of the other PR hasn't responded to any of my review comments, which is honestly a bit worrisome

In the end, having the theme available and working for Plasma 6 users should be a priority, given how many people have already switched. If we wait longer for the more substantial PR or for upstream to modify the script, we might cause a lot of people to wait for us to update before they update to Plasma 6, or even to choose an entirely different theme altogether. Therefore, we should try to get this in Nixpkgs fairly quickly

@GiggleSquid
Copy link
Contributor Author

That all sounds good to me.
If you like, I'll reformat to the original to reduce the diff?

@pluiedev
Copy link
Contributor

That all sounds good to me.

If you like, I'll reformat to the original to reduce the diff?

Please do 😺👍

@GiggleSquid GiggleSquid force-pushed the pkgs/catppuccin-kde-plasma6 branch from c55ab4b to 32fec56 Compare March 17, 2024 19:54
pkgs/by-name/op/open-scq30/package.nix Outdated Show resolved Hide resolved
@GiggleSquid GiggleSquid force-pushed the pkgs/catppuccin-kde-plasma6 branch from 32fec56 to c4a2513 Compare March 17, 2024 20:11
@GiggleSquid GiggleSquid added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Mar 17, 2024
@GiggleSquid GiggleSquid added 12.approvals: 2 This PR was reviewed and approved by two reputable people and removed 12.approvals: 1 This PR was reviewed and approved by one reputable person labels Mar 18, 2024
@wegank wegank merged commit 8de57ef into NixOS:master Mar 20, 2024
27 checks passed
@GiggleSquid GiggleSquid deleted the pkgs/catppuccin-kde-plasma6 branch March 23, 2024 20:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 10.rebuild-linux: 1 11.by: package-maintainer This PR was created by the maintainer of the package it changes 12.approvals: 2 This PR was reviewed and approved by two reputable people
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants