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

Revert "qutebrowser: 2.5.4 -> 3.0.0" #251660

Merged
1 commit merged into from Aug 26, 2023
Merged

Revert "qutebrowser: 2.5.4 -> 3.0.0" #251660

1 commit merged into from Aug 26, 2023

Conversation

ghost
Copy link

@ghost ghost commented Aug 26, 2023

Description of changes

PR #250171 was misrepresented as a version bump. The package already supported qutebrowser 2.5.4 (on non-qt6 platforms) and 3.0.0 (on qt6 platforms). That PR was actually a deletion of the non-qt6 version of qutebrowser. Commits titled "X -> Y" are for version bumps, not deletions.

I've had PRs for cross-compilation of qt5 pending and gradually merging over the past six months; they culminate in the final goal of being able to cross-compile qutebrowser (on qt5). I've been waiting for them to merge before starting on qt6. It's really frustrating to have all that work thrown away by something like #250171.

If you want to speed up deprecation of qt5, please review the cross-compilation patches and get them merged so I can do the same thing for qt6.

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.

This reverts commit 180f793.

@ghost ghost requested review from Ericson2314 and matthewbauer as code owners August 26, 2023 19:57
@github-actions github-actions bot added 6.topic: stdenv Standard environment and removed 6.topic: stdenv Standard environment labels Aug 26, 2023
@ghost
Copy link
Author

ghost commented Aug 26, 2023

@ofborg eval

@dotlambda
Copy link
Member

Version 3.0.0 still supports Qt 5.15. Can we add a qutebrowser-qt5 variant instead?

@ghost ghost merged commit a6f3b53 into NixOS:master Aug 26, 2023
@ghost ghost deleted the pr/revert/250171 branch August 26, 2023 20:49
@ghost
Copy link
Author

ghost commented Aug 26, 2023

Version 3.0.0 still supports Qt 5.15. Can we add a qutebrowser-qt5 variant instead?

One step at a time:

It's better to use stdenv.isAvailable to detect the newest usable version of QT automatically so we don't have to junk up the top-level namespace with all these extra package names. Since stdenv.isAvailable isn't merged yet we can use meta.broken in the meantime.

@ofborg ofborg bot added the 8.has: package (new) This PR adds a new package label Aug 26, 2023
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux and removed 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 labels Aug 26, 2023
@samueldr
Copy link
Member

samueldr commented Aug 26, 2023

#250171 was a package update. It was done pretty much as it should have been by one of its package maintainer.

Though maybe a day before merging wasn't much warning, I'm also not sure it warranted such warning. I'm not the one who will judge that fact.

Nixpkgs is not a hoard of old package versions.

Upstream moved to 3.0.0, thus the maintainers updated to 3.0.0. Upstream also now supports Qt 6, thus we follow upstream's supported use-case.

This self-merged revert done without giving ample time for the maintainers to chime-in is not the way to go. 52 minutes is not an appropriate amount of time for this situation. This is not a highly critical package, this is not a security issue.

@rnhmjoj
Copy link
Contributor

rnhmjoj commented Aug 27, 2023

I've just woken up to this: as samueldr said, you should at least give some time to reply before merging something like this.

Upstream now defaults to Qt6 and Qt5 is barely useful because the web engine is so outdated most websites refuse to work. Reverting the update is not ok in this case.
If Qt6 has less supported platforms than aarch64 and x86_64, that's another issue and does not directly concern me, a maintainer of qutebrowser.

The top-level qutebrowser package should default to Qt6, if you care about preserving the old backend you should copy the old expression and define a qutebrowser-qt5 variant. Also, please, do not generalise the expression to handle both, because that makes maintaining the package so much more difficult.

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
8.has: package (new) This PR adds a new package 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants