zoom-us: fix web login #76056
zoom-us: fix web login #76056
Conversation
|
@pbogdan yeah, I'll backport it, perhaps together with Zoom update |
pkgs/applications/networking/instant-messengers/zoom-us/default.nix
Outdated
Show resolved
Hide resolved
| ln -s $out/share/zoom-us/zoom $out/bin/zoom-us | ||
|
|
||
| # Do the manual Qt wrapping, because existing hook doesn't support our usecase | ||
| qtWrapperArgs+=(--prefix XDG_DATA_DIRS : "$out/share/zoom-us") |
There was a problem hiding this comment.
can use use the nix attr below for this, or does it not work? (I believe you can use placeholder)
There was a problem hiding this comment.
After inspecting the produced wrappers they're missing an entry to XDG_DATA_DIRS for $out/share which needs to be in these wrappers too. I see code in wrapQtAppsHook for that, so this confuses me.
Can you also add a comment why we need the zoom-us subdir in XDG_DATA_DIRS?
There was a problem hiding this comment.
I also believe you're wrapping in the wrong phase, can this be done in preFixup?
There was a problem hiding this comment.
I also believe you're wrapping in the wrong phase, can this be done in preFixup?
changed to postFixup, as I see there are some hooks added in preFixup in wrap-qt-setup-hook.sh. I don't know what will be the run order, but postFixup will definitely run after.
pkgs/applications/networking/instant-messengers/zoom-us/default.nix
Outdated
Show resolved
Hide resolved
pkgs/applications/networking/instant-messengers/zoom-us/default.nix
Outdated
Show resolved
Hide resolved
|
@worldofpeace thanks a lot for review, didn't know about There was a little problem with passing And the other problem comes from fact that zoom binaries live in directory, which isn't included into qt wrapper |
maintainers/scripts/update.nix
Outdated
| @@ -105,7 +105,7 @@ let | |||
| to run all update scripts for all packages that lists \`garbas\` as a maintainer | |||
| and have \`updateScript\` defined, or: | |||
|
|
|||
| % nix-shell maintainers/scripts/update.nix --argstr package garbas | |||
| % nix-shell maintainers/scripts/update.nix --argstr package gnome3 | |||
There was a problem hiding this comment.
I actually think several things aren't correct here. This should be like maintainer $maintainer_attr, and to run for a specific package below should be package. This is out of scope and has various other inaccuracy, can you drop the commit?
There was a problem hiding this comment.
no, it is real typo. Your suggested variant is already listed https://github.com/NixOS/nixpkgs/blob/master/maintainers/scripts/update.nix#L103-L111
There was a problem hiding this comment.
but I found that my suggestion was bad as well. I've changed the suggestion and verified it runs
pkgs/applications/networking/instant-messengers/zoom-us/default.nix
Outdated
Show resolved
Hide resolved
|
#75903 (comment) is reason enough for me to believe you should do this. Though this is the constant drawback with unfree software 🤣, you're never going to know what exactly it's doing easily. |
(but don't pollute environment)
|
cherried to 33bacd3 Also, updated Zoom to latest in release-19.09 |
Fixes #75903
Partially helps with #73532, because it is now possible to disable
embeddedBrowserForSSOLoginand login via browsercc @dtzWill @tadfisher @mkg20001 @pbogdan
cc @worldofpeace @bjornfor @ttuegel maybe it is possible to reuse existing wrapper, I haven't found a way.