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

prismlauncher: 8.3 -> 8.4; refactor #321851

Merged
merged 7 commits into from
Jun 27, 2024

Conversation

getchoo
Copy link
Member

@getchoo getchoo commented Jun 22, 2024

Description of changes

Changelog: https://github.com/PrismLauncher/PrismLauncher/releases/tag/8.4 (this actual changelog isn't here just yet, but will be soon)

Diff: PrismLauncher/PrismLauncher@8.3...8.4

This is a standard release with no major changes; I have gone back to refactor and improved some things while we're at it, though

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.11 Release Notes (or backporting 23.11 and 24.05 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.

@getchoo getchoo added the 11.by: upstream-developer This issue or PR was created by the developer of packaged software label Jun 22, 2024
@getchoo
Copy link
Member Author

getchoo commented Jun 22, 2024

from the pkgs/by-name check:

- Because pkgs/by-name/pr/prismlauncher exists, the attribute `pkgs.prismlauncher` must be defined like

    prismlauncher = callPackage ./../by-name/pr/prismlauncher/package.nix { /* ... */ };

  However, in this PR, a different `callPackage` is used. See the definition in pkgs/top-level/all-packages.nix:36927:

    prismlauncher = kdePackages.callPackage ../by-name/pr/prismlauncher/package.nix { };

is this not allowed when migrating to by-name? or a false positive?

@Atemu
Copy link
Member

Atemu commented Jun 22, 2024

cc @infinisil

@ofborg ofborg bot requested review from Scrumplex and Minion3665 June 22, 2024 23:43
@ofborg ofborg bot added 11.by: package-maintainer This PR was created by the maintainer of the package it changes 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 labels Jun 22, 2024
@infinisil
Copy link
Member

infinisil commented Jun 22, 2024

This should be improved in the future, but for now this isn't a false positive, see also https://github.com/NixOS/nixpkgs/blob/master/pkgs/by-name/README.md#limitations :)

getchoo added 5 commits June 22, 2024 22:37
this primarily reorders arguments and items in lists, fixes the
meta-attributes of the wrapped version of the package, tidies up some
comments, and adds new ones to better explain why we do certain things
@getchoo getchoo force-pushed the pkgs/prismlauncher/8.4 branch from 2b29afa to 98a046d Compare June 23, 2024 02:44
@getchoo
Copy link
Member Author

getchoo commented Jun 23, 2024

Rhis should be improved in the future, but for now this isn't a false positive, see also https://github.com/NixOS/nixpkgs/blob/master/pkgs/by-name/README.md#limitations :)

tysm for the clarification. guess i missed that :p

@getchoo getchoo requested a review from AndersonTorres June 23, 2024 02:45
pkgs/by-name/pr/prismlauncher-unwrapped/package.nix Outdated Show resolved Hide resolved
, ninja
, jdk17
, zlib
, qtbase
, quazip
, kdePackages
Copy link
Member

Choose a reason for hiding this comment

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

Can this be changed to qt5 or qt6?

Copy link
Member Author

Choose a reason for hiding this comment

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

it can be, but shouldn't. we're currently in the process of deprecating distribution of builds against qt 5 in distros where possible (and reasonable ofc) upstream, up until it's eventual removal

if users really want to use qt 5 still for whatever reason, i think using the standard override interface here would suffice

Copy link
Member

Choose a reason for hiding this comment

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

So, can this then be changed to qt6?

As I have seen at the website, this application does not use KDE-specific things, only a small bunch of Qt libs...

P.S.: can you point me to a specific place where the Qt5 deprecation is discussed? I have some Qt6+5 packages to deal with...

Copy link
Member Author

Choose a reason for hiding this comment

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

So, can this then be changed to qt6?

is kdePackages not qt 6? i'm a bit confused here

P.S.: can you point me to a specific place where the Qt5 deprecation is discussed? I have some Qt6+5 packages to deal with...

we're tracking this upstream at PrismLauncher/PrismLauncher#2324 with the actual removal in PrismLauncher/PrismLauncher#2174. we dropped support for our qt 5 build here in #305556

pkgs/by-name/pr/prismlauncher-unwrapped/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/pr/prismlauncher-unwrapped/package.nix Outdated Show resolved Hide resolved
getchoo added 2 commits June 23, 2024 14:43
controllerSupport and textToSpeechSupport have no effect outside of
linux and have no reason to be set; both of these work out of the box on
darwin without any intervention from us
@getchoo getchoo force-pushed the pkgs/prismlauncher/8.4 branch from 98a046d to 94ad1c3 Compare June 23, 2024 18:48
@matteo-pacini
Copy link
Contributor

Result of nixpkgs-review pr 321851 run on aarch64-darwin 1

2 packages built:
  • prismlauncher
  • prismlauncher-unwrapped

@Scrumplex Scrumplex mentioned this pull request Jun 26, 2024
13 tasks
Copy link
Member

@Atemu Atemu left a comment

Choose a reason for hiding this comment

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

Diff LGTM and I ran MC with it

@Atemu Atemu merged commit 6d7edd3 into NixOS:master Jun 27, 2024
24 checks passed
@getchoo getchoo deleted the pkgs/prismlauncher/8.4 branch June 28, 2024 00:34
Copy link
Contributor

github-actions bot commented Jul 5, 2024

Backport failed for release-24.05, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin release-24.05
git worktree add -d .worktree/backport-321851-to-release-24.05 origin/release-24.05
cd .worktree/backport-321851-to-release-24.05
git switch --create backport-321851-to-release-24.05
git cherry-pick -x dce34fd38b6cd06d2938807c74e731b81f677da6 b40318aaf71299220228c71a3b14fd1d2880eb37 88039bb875585904972c9598a681e63ed2a18263 40581e9eed5ec929a67e0c4109a8e83dbeb12f63 ea1cf5da523b1febfa5078a2c5db09fd056ffe92 3b2969b1a569e1f95ad0e51b8fdb05f76efdf1cc 94ad1c37a84a2065fe30470125d577aa5e3b3994

@Atemu
Copy link
Member

Atemu commented Jul 5, 2024

Could you explain why you think this meets the backport criteria?

@getchoo
Copy link
Member Author

getchoo commented Jul 5, 2024

this is a minor update that contains new functionality and large bug fixes with no breaking changes

regarding the changes to the expression: the only thing that really breaks here would be some of the new assertions. i would honestly consider these bugs in the packaging itself though, as there's no reason to have many of them set on platforms besides linux and allowing them to be defined but be a no-op isn't a great experience. the defaults of these overrides also remain the same, keeping impact pretty minimal

@Atemu
Copy link
Member

Atemu commented Jul 6, 2024

Hm, I'm honestly not convinced. Unless these bugs are somehow critical to the app's operation, they don't really fit the criteria.

Though even if it was critical, I'd ask you to cherry-pick a patch instead.

The refactor also hold a bit of risk to introduce unintended regressions.

It's fine and even expected to let bugs remain in stable but you should never introduce new ones if it's in any way avoidable. At least that's my view on it.

I appreciate you wanting to bring fixes to users ASAP but users of stable want stability, not speed. People who want fixes ASAP use the rolling channel.

evan-goode added a commit to unmojang/PrismLauncher that referenced this pull request Jul 20, 2024
Brings in changes from the Prism Launcher derivation(s) in nixpkgs, notably
from NixOS/nixpkgs#321851 and
NixOS/nixpkgs#303880

Signed-off-by: Evan Goode <mail@evangoo.de>
evan-goode added a commit to unmojang/PrismLauncher that referenced this pull request Jul 21, 2024
Brings in changes from the Prism Launcher derivation(s) in nixpkgs, notably
from NixOS/nixpkgs#321851 and
NixOS/nixpkgs#303880

Signed-off-by: Evan Goode <mail@evangoo.de>
evan-goode added a commit to unmojang/FjordLauncher that referenced this pull request Aug 12, 2024
Brings in changes from the Prism Launcher derivation(s) in nixpkgs, notably
from NixOS/nixpkgs#321851 and
NixOS/nixpkgs#303880

Signed-off-by: Evan Goode <mail@evangoo.de>
evan-goode added a commit to unmojang/FjordLauncher that referenced this pull request Aug 12, 2024
Brings in changes from the Prism Launcher derivation(s) in nixpkgs, notably
from NixOS/nixpkgs#321851 and
NixOS/nixpkgs#303880

Signed-off-by: Evan Goode <mail@evangoo.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 11.by: package-maintainer This PR was created by the maintainer of the package it changes 11.by: upstream-developer This issue or PR was created by the developer of packaged software
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants