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

qt5.qutebrowser: reinit at 3.0.2 #265364

Closed
wants to merge 1 commit into from
Closed

qt5.qutebrowser: reinit at 3.0.2 #265364

wants to merge 1 commit into from

Conversation

ghost
Copy link

@ghost ghost commented Nov 4, 2023

Description of changes

Our qutebrowser package was vandalized by various commits, such as c9cc3a2 and ad0bbaf, which made erroneous assertions such as "since qutebrowser 3.0.0 the derivation is only building for qt6."

This commit repairs the vandalism.

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/)
  • 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.

Our qutebrowser package has been vandalized by various commits, such
as c9cc3a2 and
ad0bbaf, which made erroneous
assertions such as "since qutebrowser 3.0.0 the derivation is only
building for qt6."

This commit repairs the vandalism.
@ghost
Copy link
Author

ghost commented Nov 4, 2023

@ofborg build qt5.qutebrowser

@ghost ghost marked this pull request as ready for review November 4, 2023 01:31
@ghost ghost requested a review from ttuegel as a code owner November 4, 2023 01:31
@ghost
Copy link
Author

ghost commented Nov 4, 2023

@ofborg eval

@ghost
Copy link
Author

ghost commented Nov 4, 2023

  • qt5.qutebrowser on aarch64-linux — Success Details

  • qt5.qutebrowser on x86_64-linux — Success Details

@dotlambda
Copy link
Member

What's wrong with #251671? That looks much simpler.

readability-lxml pykeepass stem
readability-lxml pykeepass
] ++ lib.optionals ((builtins.tryEval stem.outPath).success) [
# error: stem-1.8.2 not supported for interpreter python3.11
Copy link
Member

Choose a reason for hiding this comment

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

That was fixed by #262813.

@nrdxp
Copy link
Contributor

nrdxp commented Nov 4, 2023

This commit repairs the vandalism.

sorry but I don't think its considered vandalism to not support a browser backend which is severely outdated and probaby rife with security vulnerabilities at this point. It was a conscious decision and one recommended by upstream.

In any case, I don't think it's a good idea to support this, but if you really want to, you should probably have some sort of warning during eval that the browser backend is the insecure.

@rnhmjoj
Copy link
Contributor

rnhmjoj commented Nov 5, 2023

Our qutebrowser package was vandalized by various commits, such as c9cc3a2 and ad0bbaf

Now this is getting ridiculous. I don't really understand how could anyone get so worked up on something so trivial, but please drop this.

As multiple persons have already told you, it didn't make sense to support the old backend active as it is both outdated (even basic web apps such as discourse refuse to work) and insecure. Also, having two variants of qutebrowser in the same expression made it more complicated and it was a hassle to maintain.

IIRC you want to keep Qt5 because Qt6 does not cross compile, right? I would tell you to work on fixing Qt6 instead, but I guess it's ok to keep a copy of the package with Qt pinned at Qt 5.15, like it's usually done for libraries like boost, etc. Downgrading the default package, however it's not an option.
Also, I think you should be maintaing this copy of the package, or at least add yourself to the qutebrowser maintainers.

Anyway, I don't understand the purpose of this PR considering that #251671 does exactly the same thing, sans the bit about the qt5. namespace. This, by the way, has always been used for collecting the Qt modules, so moving qutebrowser there doesn't make sense to me.

@dotlambda
Copy link
Member

These commits were merged in #264965 without making requested changes.

@dotlambda dotlambda closed this Nov 5, 2023
@dotlambda dotlambda mentioned this pull request Nov 6, 2023
13 tasks
@zimbatm
Copy link
Member

zimbatm commented Nov 7, 2023

@amjoseph-nixpkgs nixpkgs is a shared resource, and we have to find ways to work together. For this to work it's important that:

  1. we don't assume people are doing things maliciously. Using terms like "vandalize" is antagonizing.
  2. when feedback is given, some form of acknowledgement of understanding is given back. Nixpkgs is a big negotiation exercise and it doesn't work if some actors are doing things one-sided.
  3. this also goes for self-merging PRs. Some sort of shared understanding needs to be developed.

If there are things frustrating you then let's find ways to make it smoother. Please DM me if you would like to discuss this privately.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants