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

pavucontrol-qt: use libsForQt5.callPackage #31009

Merged
merged 1 commit into from Nov 2, 2017
Merged

pavucontrol-qt: use libsForQt5.callPackage #31009

merged 1 commit into from Nov 2, 2017

Conversation

jokogr
Copy link
Contributor

@jokogr jokogr commented Oct 30, 2017

Motivation for this change

I started using pavucontrol-qt on xmonad and noticed that it was pulling a previous version of Qt (5.9.1 instead of 5.9.2).

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option build-use-sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

orivej
orivej approved these changes Oct 30, 2017
View reviewed changes
Copy link
Contributor

@orivej orivej left a comment

Choose a reason for hiding this comment

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

AFAIK this is a correct change, but it would not have prevented you from having the issue you describe: qt5.callPackage will ensure that your package does not depend on Qt 5.6 and Qt 5.9 simultaneously, but it will not help you when you install something that depends on Qt 5.9.1 with nix-env, update the channel which is now on Qt 5.9.2, and perform nix-env -i pavucontrol-qt.

@lukateras
Copy link
Member

I think libsFоrQt5.callPackage is typically used over pkgs.qt5.callPackage.

peterhoeg
peterhoeg reviewed Nov 2, 2017
View reviewed changes
pkgs/desktops/lxqt/default.nix Outdated
@@ -45,7 +45,7 @@ let
lxqt-qtplugin = callPackage ./core/lxqt-qtplugin { };
lxqt-session = callPackage ./core/lxqt-session { };
lxqt-sudo = callPackage ./core/lxqt-sudo { };
pavucontrol-qt = callPackage ./core/pavucontrol-qt { };
pavucontrol-qt = pkgs.qt5.callPackage ./core/pavucontrol-qt { };
Copy link
Member

Choose a reason for hiding this comment

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

As mentioned by @yegortimoshenko, this should be:
libsForQt5.callPackage ./core/pavucontrol-qt { };

@jokogr
Copy link
Contributor Author

jokogr commented Nov 2, 2017

@peterhoeg I have made the necessary change and rebased against the current master.

@orivej @yegortimoshenko @peterhoeg thanks for the reviews!

@peterhoeg
Copy link
Member

And it still builds and runs properly?

@jokogr
Copy link
Contributor Author

jokogr commented Nov 2, 2017

@peterhoeg it seems that it uses the binary cache, so I suppose libsFоrQt5.callPackage is equivalent to plain callPackage.

I suggest to keep it with callPackage, but expose the exact dependencies as arguments (original derivation requires qt5, not qtbase etc.)

orivej
orivej approved these changes Nov 2, 2017
View reviewed changes
Copy link
Contributor

@orivej orivej left a comment

Choose a reason for hiding this comment

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

I've rebased this on top of 9258056

@jokogr
Copy link
Contributor Author

jokogr commented Nov 2, 2017 •
edited

Modified it a bit and rebased it, too, the new version which appeared today, works here!

@orivej orivej changed the title pavucontrol-qt: use qt5.callPackage pavucontrol-qt: use libsForQt5.callPackage Nov 2, 2017
@orivej orivej merged commit 86b3c73 into NixOS:master Nov 2, 2017
@orivej
Copy link
Contributor

orivej commented Nov 2, 2017

Thank you!

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

@peterhoeg peterhoeg peterhoeg left review comments

@orivej orivej orivej approved these changes

Assignees
No one assigned
Labels
9.needs: reporter feedback 10.rebuild-darwin: 0 10.rebuild-linux: 0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@jokogr @lukateras @peterhoeg @orivej @GrahamcOfBorg