-
-
Notifications
You must be signed in to change notification settings - Fork 15k
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
quaternion: update, libquotient: init #84160
Conversation
c064046
to
4a043cb
Compare
@GrahamcOfBorg eval |
I thought it was just a rename. Could you please swtich the
Reference: quotient-im/libQuotient@3d2b359 Perhaps you should add that link to the commit message. |
pkgs/applications/networking/instant-messengers/quaternion/default.nix
Outdated
Show resolved
Hide resolved
Error was:
Note that this removes quaternion-git, which should probably point to quaternion. |
4a043cb
to
70a601f
Compare
25530d1
to
7dbf441
Compare
Eh, I don't think there's a clean way to split this into two commits. It should probably be squashed to one but I've forced-pushed this enough times for one night. nixpkgs-review rev HEAD passes. |
I'm not sure what to do about removing |
It's OK don't mind it.
I'd apply this patch, for both quaternion-git and libqmatrixclient: diff --git i/pkgs/top-level/aliases.nix w/pkgs/top-level/aliases.nix
index 88e7d4d9e84..f77bf5374c8 100644
--- i/pkgs/top-level/aliases.nix
+++ w/pkgs/top-level/aliases.nix
@@ -33,6 +33,8 @@ in
### Deprecated aliases - for backward compatibility
mapAliases ({
+ quaternion-git = throw "quaternion-git has been removed in favor of the stable version 'quaternion'"; # added 2020-04-04
+ libqmatrixclient = throw "libqmatrixclient was renamed to libQuotient"; # added 2020-04-04
PPSSPP = ppsspp; # added 2017-10-01
QmidiNet = qmidinet; # added 2016-05-22
accounts-qt = libsForQt5.accounts-qt; # added 2015-12-19 With the commit message:
And another commit removing libqmatrixclient with the message:
|
@colemickens Also, please add yourself as maintainer to |
df73aaa
to
49d8c7d
Compare
There we go, all of these commits look good. I think I've addressed all feedback. |
I'm not sure this works though. Can an existing user validate if this works in non-Sway desktops? The main widget is just always white, so even after it syncs there is no chat contents. |
Seems related:
|
I feel like this is missing some usual wrapper that I'm expecting for a qt5 app. I'll dig into it more. |
I pulled down the patch a few days ago and have been running it here on KDE since without any issues. |
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.
@@ -0,0 +1,24 @@ | |||
{ stdenv, fetchFromGitHub, cmake, qtbase, qtmultimedia }: | |||
|
|||
stdenv.mkDerivation rec { |
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.
stdenv.mkDerivation rec { | |
mkDerivation rec { |
@@ -0,0 +1,24 @@ | |||
{ stdenv, fetchFromGitHub, cmake, qtbase, qtmultimedia }: |
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.
{ stdenv, fetchFromGitHub, cmake, qtbase, qtmultimedia }: | |
{ mkDerivation, lib, fetchFromGitHub, cmake, qtbase, qtmultimedia }: |
|
||
nativeBuildInputs = [ cmake ]; | ||
|
||
meta = with stdenv.lib; { |
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.
meta = with stdenv.lib; { | |
meta = with lib; { |
buildInputs = [ qtbase qtmultimedia qtquickcontrols qtkeychain library libsecret ]; | ||
|
||
nativeBuildInputs = [ cmake qttools ]; | ||
stdenv.mkDerivation rec { |
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.
stdenv.mkDerivation rec { | |
mkDerivation rec { |
49d8c7d
to
70f9dd8
Compare
Thanks @Emantor, that did the trick, working for me with Wayland/sway. (It did crash fairly easily, but I'm going to ignore that, I know there's other Qt5 updates coming and I know they include Wayland improvements.) I think this is good to go now. |
Should be |
Hmm. Why should it be I'm not really a fan of the mixed-case name myself, but I figured it was better to call that package what it's called upstream. I think it prevents cases of someone coming along and saying "oh, where is libQuotient, it's not here, I'll package it... oh wait it's already packaged as libquotient", which just happened to me with Is there a convention here we follow or am I reading the situation wrong? Thanks! |
Yea I too think it's not that important @colemickens . The best advice for a "wait libfoo is not packaged?" is to use |
70f9dd8
to
bea3e15
Compare
To avoid exactly the problem you mention, the convention is that they are all lower case. I mean, the software referenced here "Quaternion" is still called "quaternion". |
@colemickens Though I slightly disagree with @peterhoeg, he has commit access whereas I don't :). I'm confident you will finally get your PR merged once you'd only rename |
The nixpkgs manual actually mentions that attributes in |
Hi. I don't mind updating capitalization, just wanted to know for guidance going forward. I do appreciate the review and will fix the capitalization later today and push. Knowing the manual encourages lower-cased attributes gives me the guidance I was looking for. :) Regarding API/ABI compatibility... I don't really know at all. I can try to find out. |
bea3e15
to
9498751
Compare
Alright, this should be closer. Thanks for the guidance. |
pkgs/applications/networking/instant-messengers/quaternion/default.nix
Outdated
Show resolved
Hide resolved
9498751
to
be5220d
Compare
aliases: throw messages for libqmatrixclient and quaternion-git
be5220d
to
3ceefd2
Compare
Motivation for this change
It seems like
quaternion
(a matrix client) has moved fromlibqmatrixclient
tolibQuotient
.This PR includes two commits:
Things done
sandbox
innix.conf
on non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
./result/bin/
)nix path-info -S
before and after)