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.

Sign up for GitHub

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

Jump to bottom

qutebrowser-qt5: replace qt5.qutebrowser #251671

Merged
merged 2 commits into from
Nov 24, 2023
Merged

qutebrowser-qt5: replace qt5.qutebrowser #251671

merged 2 commits into from
Nov 24, 2023

Conversation

dotlambda
Copy link
Member

Description of changes

see #251660

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • 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/)
  • 23.11 Release Notes (or backporting 23.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.

@dotlambda dotlambda requested review from rnhmjoj and a user August 26, 2023 21:00
@dotlambda dotlambda mentioned this pull request Aug 26, 2023
Merged
ghost
ghost previously requested changes Aug 26, 2023
View reviewed changes
Copy link

@ghost ghost left a comment •
edited by ghost
Loading

Choose a reason for hiding this comment

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

This package is already named qutebrowser.

We already have a package named qutebrowser-qt6.

There is no need to pollute top-level/all-packages.nix with yet-more decorated package names. Prior to #250171 we were automatically detecting and using the newest available version of QT for the qutebrowser package. That's the right way to do it.

@dotlambda
Copy link
Member Author

Prior to #250171 we were automatically detecting and using the newest available version of QT for the qutebrowser package.

We were? Afaict we were simply using Qt5 by default.

@dotlambda
Copy link
Member Author

While I agree that it's annoying some people would have to replace qutebrowser with qutebrowser-qt5 after this change I see a benefit in having qutebrowser be the default that upstream suggests, i.e. Qt6. Until #245322 is ready we sadly can't choose the correct version for people.

@ofborg ofborg bot requested review from jagajaga, ebzzry and nrdxp August 26, 2023 21:50
@rnhmjoj
Copy link
Contributor

rnhmjoj commented Aug 27, 2023 •
edited
Loading

We already have a package named qutebrowser-qt6.

qutebrowser should default to Qt6, because that's what upstream recommends.

There is no need to pollute top-level/all-packages.nix with yet-more decorated package names.

Agreed, and that's why qutebrowser-qt6 should be removed as well, it should not have been introduced in the first place. Unfortunately the upgrade to Qt6 took so long that people got impatient...

@nrdxp
Copy link
Contributor

nrdxp commented Aug 28, 2023

If we care about not polluting the top-level we do have the option of nesting the qt5 variant inside the top-level derivation attribute somewhere, although I'm not sure if there is any precedent for that elsewhere.

thiagokokada
thiagokokada reviewed Sep 28, 2023
View reviewed changes
Copy link
Contributor

@thiagokokada thiagokokada left a comment

Choose a reason for hiding this comment

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

In general this looks good to me, but I think this kinda of change (defaulting to Qt6 instead Qt5) should have a entry in release notes.

@dotlambda dotlambda mentioned this pull request Nov 4, 2023
Closed
13 tasks
@dotlambda dotlambda mentioned this pull request Nov 7, 2023
Merged
13 tasks
@dotlambda dotlambda changed the title qutebrowser-qt5: init at 3.0.0 qutebrowser-qt5: replace qt5.qutebrowser Nov 7, 2023
@dotlambda dotlambda dismissed ghost ’s stale review November 7, 2023 19:00

The qutebrowser maintainers agree that the Qt 6 version should be the default.

@Artturin Artturin reopened this Nov 8, 2023
@ghost
Copy link

ghost commented Nov 8, 2023

This pr does not change any defaults

Then why was my review deleted with "The qutebrowser maintainers agree that the Qt 6 version should be the default." as the justification?

@Artturin
Copy link
Member

Artturin commented Nov 8, 2023

Which is worse, a top-level attr or a non qt package in the qt5 set

@ghost
Copy link

ghost commented Nov 8, 2023 •
edited by ghost
Loading

Which is worse, a top-level attr or a non qt package in the qt5 set

We have plenty of packages besides python in the pythonPackages set.

Indeed, the difference between python310Packages and python311Packages is precisely which version of python your python-using package uses.

@Artturin
Copy link
Member

Artturin commented Nov 8, 2023

Which is worse, a top-level attr or a non qt package in the qt5 set

We have plenty of packages besides python in the pythonPackages set.

Indeed, the difference between python310Packages and python311Packages is precisely which version of python your python-using package uses.

Python programs are not supposed to be in pythonPackages, only modules.

@ghost ghost dismissed their stale review November 8, 2023 05:10

this is not the hill to die on

Artturin
Artturin reviewed Nov 8, 2023
View reviewed changes
pkgs/top-level/all-packages.nix
inherit (__splicedPackages.qt6Packages) qtbase qtwebengine wrapQtAppsHook qtwayland;
};

qutebrowser-qt5 = callPackage ../applications/networking/browsers/qutebrowser {
Copy link
Member

Choose a reason for hiding this comment

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

It needs to have -qt5 appended to the pname

nixpkgs/pkgs/applications/networking/browsers/qutebrowser/default.nix

Line 29 in 40e5c02

pname = "qutebrowser";

Because nix-env uses name , idk what it does when 2 derivations have the same pname and version

Copy link
Member Author

Choose a reason for hiding this comment

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

I honestly don't care, nix-env shouldn't be used. This might be fixed by marking the Qt 5 version insecure though.

Copy link
Member

Choose a reason for hiding this comment

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

Added it

@Artturin
Copy link
Member

Artturin commented Nov 8, 2023 •
edited
Loading

@amjoseph-nixpkgs I don't think there's anything here (functionally) that you should be against since it's just moving the attr.

We have done -X suffixed attrs before too, though usually they're done via a callPackage arguments.

@ghost
Copy link

ghost commented Nov 8, 2023

Indeed, the difference between python310Packages and python311Packages is precisely which version of python your python-using package uses.

Python programs are not supposed to be in pythonPackages, only modules.

Can you imagine how bonkers it would be if every package that depended on python had three top-level variations?

  • pytorch
  • pytorch-python310
  • pytorch-python311

... and people had to deal with the version that works for them being renamed from pytorch to pytorch-python310 while the python311 bugs in pytorch got sorted out?

ghost
ghost reviewed Nov 8, 2023
View reviewed changes
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

We should mark qt6Packages.qtbase as meta.broken if stdenv.buildPlatform != stdenv.hostPlatform.

pkgs/top-level/all-packages.nix
Comment on lines +35047 to +35051
inherit (__splicedPackages.qt6Packages) qtbase qtwebengine wrapQtAppsHook qtwayland;
};

qutebrowser-qt5 = callPackage ../applications/networking/browsers/qutebrowser {
inherit (__splicedPackages.libsForQt5) qtbase qtwebengine wrapQtAppsHook qtwayland;
Copy link

Choose a reason for hiding this comment

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

Suggested change
inherit (__splicedPackages.qt6Packages) qtbase qtwebengine wrapQtAppsHook qtwayland;
};
qutebrowser-qt5 = callPackage ../applications/networking/browsers/qutebrowser {
inherit (__splicedPackages.libsForQt5) qtbase qtwebengine wrapQtAppsHook qtwayland;
inherit (if stdenv.buildPlatform == stdenv.hostPlatform then __splicedPackages.qt6Packages else __splicedPackages.libsForQt5)
qtbase qtwebengine wrapQtAppsHook qtwayland;
};

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think we should make it intransparent to the user that they're using a different Qt version when cross-compiling.

@dotlambda
Copy link
Member Author

We should mark qt6Packages.qtbase as meta.broken if stdenv.buildPlatform != stdenv.hostPlatform.

That can be done in a separate PR.

@dotlambda
Copy link
Member Author

Can you imagine how bonkers it would be if every package that depended on python had three top-level variations?

It would be. That's why you have to use override if you want to use a different Python version with a Python application. I think that would be a perfectly fine solution with qutebrowser too, but I think qutebrowser-qt5 can be added for your convenience.

@Artturin
Copy link
Member

Artturin commented Nov 11, 2023 •
edited
Loading

https://github.com/NixOS/nixpkgs/blob/master/pkgs/top-level/qt5-packages.nix and https://github.com/NixOS/nixpkgs/blob/master/pkgs/top-level/qt6-packages.nix seems to be the places for packages like this?

@dotlambda
Copy link
Member Author

https://github.com/NixOS/nixpkgs/blob/master/pkgs/top-level/qt5-packages.nix and https://github.com/NixOS/nixpkgs/blob/master/pkgs/top-level/qt6-packages.nix seems to be the places for packages like this?

No, that's for Qt libraries.

@dotlambda
Copy link
Member Author

@Artturin Do you think this can be merged?

@ofborg ofborg bot requested a review from rnhmjoj November 20, 2023 18:01
@rnhmjoj
Copy link
Contributor

rnhmjoj commented Nov 21, 2023

It seems to be working normally, though I noticed this warning:

WARNING: QtWebEngine version mismatch - unexpected behavior might occur, please open a bug about this.
  Early version: QtWebEngine 5.15.4, based on Chromium 87.0.4280.144 (from PyQt)
  Real version:  QtWebEngine 5.15.14, based on Chromium 87.0.4280.144

@Artturin Artturin merged commit 7cc4f72 into NixOS:master Nov 24, 2023
23 checks passed
@github-actions github-actions bot mentioned this pull request Nov 24, 2023
Merged
1 task
@github-actions GitHub Actions
Copy link
Contributor

Successfully created backport PR for release-23.11:

@rnhmjoj
Copy link
Contributor

rnhmjoj commented Nov 24, 2023

So... is the "QtWebEngine version mismatch" warning not important?

@Artturin
Copy link
Member

So... is the "QtWebEngine version mismatch" warning not important?

IDK, it was happening before so /shrug.

@dotlambda dotlambda deleted the qutebrowser-qt5 branch November 24, 2023 20:46
@trofi
Copy link
Contributor

trofi commented Jan 25, 2024

Minor eval failure fix around tests.cross.sanity: #283737

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thiagokokada thiagokokada thiagokokada approved these changes

@Artturin Artturin Artturin approved these changes

@jagajaga jagajaga Awaiting requested review from jagajaga

@ebzzry ebzzry Awaiting requested review from ebzzry

@nrdxp nrdxp Awaiting requested review from nrdxp

@ttuegel ttuegel Awaiting requested review from ttuegel ttuegel is a code owner

@rnhmjoj rnhmjoj Awaiting requested review from rnhmjoj

Assignees
No one assigned
Labels
6.topic: qt/kde 8.has: clean-up 8.has: package (new) 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 10.rebuild-linux: 1-10 10.rebuild-linux: 1 11.by: package-maintainer 12.approvals: 1 12.approved-by: package-maintainer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@dotlambda @rnhmjoj @nrdxp @Artturin @trofi @thiagokokada @wegank