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

zoom-us: 2.2.128100.0627 -> 2.2.128200.0702 #42922

Merged
merged 1 commit into from Jul 5, 2018
Merged

zoom-us: 2.2.128100.0627 -> 2.2.128200.0702 #42922

merged 1 commit into from Jul 5, 2018

Conversation

tadfisher
Copy link
Contributor

Motivation for this change

Fix #41203. The zoom binary loads ./libturbojpeg.so from its CWD, so link the lib and change the CWD in the wrapper.

Also adds libpulseaudio and pciutils to runtime paths after noticing these are used.

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 nox --run "nox-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)
  • Fits CONTRIBUTING.md.

tadfisher
tadfisher commented Jul 3, 2018
View reviewed changes
pkgs/applications/networking/instant-messengers/zoom-us/default.nix Outdated
export LD_LIBRARY_PATH="${stdenv.lib.makeLibraryPath [ libpulseaudio ]}"${"\\\${LD_LIBRARY_PATH:+':'}"}"\$LD_LIBRARY_PATH"
cd $packagePath
exec ./zoom "\$@"
EOF
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If there's a way to do this with makeWrapper, please let me know.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(or if it's possible to patch the binary somehow)

Copy link
Contributor

Choose a reason for hiding this comment

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

You seem to want to do (from makeWrapper.sh's source):

# --run   COMMAND   : run command before the executable
#                     The command can push extra flags to a magic list variable
#                     extraFlagsArray, which are then added to the invocation
#                     of the executable

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Awesome, I can't believe I missed that!

@GrahamcOfBorg
Copy link

No attempt on aarch64-linux (full log)

The following builds were skipped because they don't evaluate on aarch64-linux: zoom-us

Partial log (click to expand)


a) For `nixos-rebuild` you can set
  { nixpkgs.config.allowUnfree = true; }
in configuration.nix to override this.

b) For `nix-env`, `nix-build`, `nix-shell` or any other Nix command you can add
  { allowUnfree = true; }
to ~/.config/nixpkgs/config.nix.

@GrahamcOfBorg
Copy link

No attempt on x86_64-linux (full log)

The following builds were skipped because they don't evaluate on x86_64-linux: zoom-us

Partial log (click to expand)


a) For `nixos-rebuild` you can set
  { nixpkgs.config.allowUnfree = true; }
in configuration.nix to override this.

b) For `nix-env`, `nix-build`, `nix-shell` or any other Nix command you can add
  { allowUnfree = true; }
to ~/.config/nixpkgs/config.nix.

@tadfisher
Copy link
Contributor Author

cc maintainers: @danbst

@tadfisher
Copy link
Contributor Author

@coretemp Updated. Also made libpulseaudio optional, and used the runtimeDependencies attribute that's respected by autoPatchelfHook.

@GrahamcOfBorg
Copy link

No attempt on aarch64-linux (full log)

The following builds were skipped because they don't evaluate on aarch64-linux: zoom-us

Partial log (click to expand)


a) For `nixos-rebuild` you can set
  { nixpkgs.config.allowUnfree = true; }
in configuration.nix to override this.

b) For `nix-env`, `nix-build`, `nix-shell` or any other Nix command you can add
  { allowUnfree = true; }
to ~/.config/nixpkgs/config.nix.

@GrahamcOfBorg
Copy link

No attempt on x86_64-linux (full log)

The following builds were skipped because they don't evaluate on x86_64-linux: zoom-us

Partial log (click to expand)


a) For `nixos-rebuild` you can set
  { nixpkgs.config.allowUnfree = true; }
in configuration.nix to override this.

b) For `nix-env`, `nix-build`, `nix-shell` or any other Nix command you can add
  { allowUnfree = true; }
to ~/.config/nixpkgs/config.nix.

@domenkozar domenkozar merged commit 14555ea into NixOS:master Jul 5, 2018
@danbst
Copy link
Contributor

danbst commented Jul 7, 2018

I can't start zoom now

zoom started.
qt.qpa.plugin: Could not find the Qt platform plugin "xcb" in ""
This application failed to start because no Qt platform plugin could be initialized. Reinstalling the application may fix this problem.

I had same problem in past and fixed it by supplying qt.conf:

https://github.com/tadfisher/nixpkgs/blob/16463d728b81f159e3b5f4eaae241841b227d86f/pkgs/applications/networking/instant-messengers/zoom-us/default.nix#L83-L86

Maybe this is because I use Gnome Shell as desktop?

danbst
danbst reviewed Jul 7, 2018
View reviewed changes
pkgs/applications/networking/instant-messengers/zoom-us/default.nix
makeWrapper $packagePath/zoom $out/bin/zoom-us \
--prefix PATH : "${procps}/bin"
--prefix PATH : "${makeBinPath [ pciutils procps ]}" \
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is pciutils added to runtime PATH?

danbst
danbst reviewed Jul 7, 2018
View reviewed changes
pkgs/applications/networking/instant-messengers/zoom-us/default.nix
@@ -24,11 +30,17 @@ in stdenv.mkDerivation {
buildInputs = [
dbus glib libGL libX11 libXfixes libuuid libxcb qtbase qtdeclarative
qtlocation qtquickcontrols2 qtscript qtwebchannel qtwebengine
libjpeg_turbo pciutils procps
Copy link
Contributor

Choose a reason for hiding this comment

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

I think pciutils and procps shouldn't be mentioned here. We specify them as build inputs as arguments in package args, but those shouldn't be discovered later by build script - we refer to those explicitly.

IMO buildInputs are formed from packages that are used implicitly by build script. pciutils and procps are noop here.

@danbst
Copy link
Contributor

danbst commented Jul 7, 2018

Quoting myself:

I had same problem in past and fixed it by supplying qt.conf:

I've checked that qt.conf doesn't fix the issue. Probably those (libxcb.so, for example) should be present on LD_LIBRARY_PATH

@tadfisher tadfisher mentioned this pull request Jul 7, 2018
Merged
9 tasks
@danbst danbst mentioned this pull request Jul 11, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danbst danbst danbst left review comments

@coretemp coretemp coretemp left review comments

Assignees
No one assigned
Labels
10.rebuild-darwin: 0 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@tadfisher @GrahamcOfBorg @danbst @coretemp @domenkozar