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

quassel: Fix use of mkDerivation #67746

Merged
merged 1 commit into from Oct 1, 2019
Merged

quassel: Fix use of mkDerivation #67746

merged 1 commit into from Oct 1, 2019

Conversation

samueldr
Copy link
Member

The with stdenv; would override the mkDerivation to be the regular
one, instead of the libsForQt5 one.

This simply removes the dangerous use of the all-encompassing with,
and prefers using a more precise inherit for lib.

Motivation for this change
Things done
  • ☑️ Tested using sandboxing (nix.useSandbox on NixOS, or option 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 nix-review --run "nix-review wip"
  • ☑️ Tested execution of all binary files (usually in ./result/bin/)
  • ☑️ Determined the impact on package closure size (by running nix path-info -S before and after)
  • ☑️ Ensured that relevant documentation is up to date
  • ☑️ Fits CONTRIBUTING.md.

@samueldr samueldr mentioned this pull request Aug 30, 2019
Closed
10 tasks
@ofborg ofborg bot requested review from Phreedom and ttuegel August 30, 2019 04:43
@samueldr samueldr force-pushed the pkgs/quassel/fix-wrapQtAppsHook branch from e302d7a to 901984c Compare August 30, 2019 05:00
@worldofpeace
Copy link
Contributor

There's already a wrapProgram call happening in preFixup which will prevent the client from being wrapped.

lib.optionalString buildClient ''
  wrapProgram "$out/bin/quassel${lib.optionalString client "client"}" \
    --prefix GIO_EXTRA_MODULES : "${dconf}/lib/gio/modules"
'';

Interesting that it needs dconf, we could add this to qtWrapperArgs.

jonringer
jonringer approved these changes Aug 30, 2019
View reviewed changes
Copy link
Contributor

@jonringer jonringer left a comment •
edited

Choose a reason for hiding this comment

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

nix-review passes on NixOS
diff LGTM
binaries seem to work (although double wrapped)

[3 built, 123 copied (297.0 MiB), 49.8 MiB DL]
https://github.com/NixOS/nixpkgs/pull/67746
2 package were build:
quassel quasselClient

worldofpeace
worldofpeace previously requested changes Aug 30, 2019
View reviewed changes
Copy link
Contributor

@worldofpeace worldofpeace left a comment

Choose a reason for hiding this comment

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

It does appear to be wrapped, but it's out of luck and ordering. We need to fix #67746 (comment).

@samueldr @worldofpeace
quassel: Fix use of mkDerivation
c52b5b8 
The `with stdenv;` would override the `mkDerivation` to be the regular
one, instead of the libsForQt5 one.

This simply removes the dangerous use of the all-encompassing `with`,
and prefers using a more precise inherit for `lib`.

See NixOS#65399

Co-authored-by: worldofpeace <worldofpeace@protonmail.ch>
@worldofpeace worldofpeace force-pushed the pkgs/quassel/fix-wrapQtAppsHook branch from 901984c to c52b5b8 Compare October 1, 2019 05:23
@worldofpeace worldofpeace merged commit 0daea5f into NixOS:master Oct 1, 2019
@worldofpeace
Copy link
Contributor

backported in 0fc13aa

@samueldr samueldr deleted the pkgs/quassel/fix-wrapQtAppsHook branch October 1, 2019 17:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ttuegel ttuegel ttuegel left review comments

@jonringer jonringer jonringer approved these changes

@worldofpeace worldofpeace worldofpeace left review comments

@Phreedom Phreedom Awaiting requested review from Phreedom

Assignees
No one assigned
Labels
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
4 participants
@samueldr @worldofpeace @ttuegel @jonringer