-
-
Notifications
You must be signed in to change notification settings - Fork 14.6k
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
telegram-desktop: make webkitgtk dependency optional #353243
Conversation
01fa302
to
3c010d4
Compare
Result of 1 package failed to build:
3 packages built:
|
3c010d4
to
e9327e9
Compare
e9327e9
to
c8e55aa
Compare
pkgs/applications/networking/instant-messengers/telegram/telegram-desktop/with-webkit.nix
Outdated
Show resolved
Hide resolved
c8e55aa
to
23021c4
Compare
''; | ||
postFixup = '' | ||
mkdir -p $out/bin | ||
makeBinaryWrapper {${telegram-desktop},$out}/bin/${telegram-desktop.meta.mainProgram} \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It occurred to me: this is double wrapping right? Maybe we should follow the route of having a telegram-desktop-unwrapped
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it is... I'm not sure whether it would be trivial to get Qt wrapper args in a separate derivation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Me neither, we can ignore the aesthetic issue with double wrapping for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Surprsingly, I actually managed to get it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would this be ok?
Another idea is to copy the unwrapped binary instead of depending on the non-webkit telegram-desktop package.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My idea is to rename the original tdesktop package to tdesktop-unwrapped (minus the wrapper part), then create a new tdesktop package for the wrapper, with the argument withWebkitgtk.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm... That would mean each override has to provide two packages (unwrapped and wrapped)? And those sitting in by-name will get -unwrapped in their folder names?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not necessarily, as in NickCao@303fc78
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, ok. Will you submit it? kotato looks broken though as it switches it from Qt 5 to Qt 6 which won't work...
P.S. reading the diff I noticed I forgot to remove kAudioObjectPropertyElementMain -> kAudioObjectPropertyElementMaster replacement which shouldn't be needed with the new sdk...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed, see #353524
23021c4
to
cae6e4f
Compare
cae6e4f
to
92a8c64
Compare
Things done
nix.conf
? (See Nix manual)sandbox = relaxed
sandbox = true
nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)Add a 👍 reaction to pull requests you find important.