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.

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

Merged
merged 2 commits into from Apr 28, 2020

Conversation

colemickens
Copy link
Member

Motivation for this change

It seems like quaternion (a matrix client) has moved from libqmatrixclient to libQuotient.

This PR includes two commits:

  • libQuotient: init at 0.5.3.2
  • quaternion: 0.0.9.4c -> 0.0.9.4e
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • 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 nixpkgs-review --run "nixpkgs-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.

@doronbehar
Copy link
Contributor

@GrahamcOfBorg eval

@doronbehar
Copy link
Contributor

Has moved from libqmatrixclient to libQuotient.

I thought it was just a rename. Could you please swtich the libQuotient: init commit to a:

libqmatrixclient: rename to libQuotient ?

Reference: quotient-im/libQuotient@3d2b359

Perhaps you should add that link to the commit message.

pkgs/top-level/all-packages.nix Outdated Show resolved Hide resolved
pkgs/top-level/all-packages.nix Outdated Show resolved Hide resolved
pkgs/top-level/all-packages.nix Outdated Show resolved Hide resolved
@Emantor
Copy link
Member

Emantor commented Apr 3, 2020

Error was:

error: while evaluating 'callPackageWith' at /home/phoenix/.cache/nixpkgs-review/pr-84160/nixpkgs/lib/customisation.nix:117:35, called from /home/phoenix/.cache/nixpkgs-review/pr-84160/nixpkgs/pkgs/top-level/all-packages.nix:1234:12:
while evaluating 'makeOverridable' at /home/phoenix/.cache/nixpkgs-review/pr-84160/nixpkgs/lib/customisation.nix:67:24, called from /home/phoenix/.cache/nixpkgs-review/pr-84160/nixpkgs/lib/customisation.nix:121:8:
undefined variable 'version' at /home/phoenix/.cache/nixpkgs-review/pr-84160/nixpkgs/pkgs/applications/networking/instant-messengers/quaternion/default.nix:22:14

Note that this removes quaternion-git, which should probably point to quaternion.

@colemickens
Copy link
Member Author

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.

@colemickens
Copy link
Member Author

I'm not sure what to do about removing quaternion-git. Do I do that in top-level/aliases.nix ?

@doronbehar
Copy link
Contributor

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.

It's OK don't mind it.

I'm not sure what to do about removing quaternion-git. Do I do that in top-level/aliases.nix ?

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:

aliases: throw messages for libqmatrixclient and quaternion-git

And another commit removing libqmatrixclient with the message:

libqmatrixclient: remove at 0.4.2.1 & 0.5.2

@doronbehar
Copy link
Contributor

@colemickens Also, please add yourself as maintainer to libQuotient.

@colemickens colemickens force-pushed the nixpkgs-quaternion branch 2 times, most recently from df73aaa to 49d8c7d Compare April 10, 2020 01:39
@colemickens
Copy link
Member Author

There we go, all of these commits look good. I think I've addressed all feedback.

@colemickens
Copy link
Member Author

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.

@colemickens
Copy link
Member Author

Seems related:

Rendering QML with QQuickWidget
qrc:/qml/Timeline.qml:4:1: module "QtQuick.Layouts" is not installed
qrc:/qml/Timeline.qml:3:1: module "QtQuick.Controls.Styles" is not installed
qrc:/qml/Timeline.qml:1:1: module "QtQuick" is not installed
qrc:/qml/Timeline.qml:2:1: module "QtQuick.Controls" is not installed
qrc:/qml/Timeline.qml:4:1: module "QtQuick.Layouts" is not installed
qrc:/qml/Timeline.qml:3:1: module "QtQuick.Controls.Styles" is not installed
qrc:/qml/Timeline.qml:1:1: module "QtQuick" is not installed
qrc:/qml/Timeline.qml:2:1: module "QtQuick.Controls" is not installed
qrc:/qml/Timeline.qml:4:1: module "QtQuick.Layouts" is not installed
qrc:/qml/Timeline.qml:3:1: module "QtQuick.Controls.Styles" is not installed
qrc:/qml/Timeline.qml:1:1: module "QtQuick" is not installed
qrc:/qml/Timeline.qml:2:1: module "QtQuick.Controls" is not installed
qrc:/qml/Timeline.qml:4:1: module "QtQuick.Layouts" is not installed
qrc:/qml/Timeline.qml:3:1: module "QtQuick.Controls.Styles" is not installed
qrc:/qml/Timeline.qml:1:1: module "QtQuick" is not installed
qrc:/qml/Timeline.qml:2:1: module "QtQuick.Controls" is not installed

@colemickens
Copy link
Member Author

I feel like this is missing some usual wrapper that I'm expecting for a qt5 app. I'll dig into it more.

@peterhoeg
Copy link
Member

I pulled down the patch a few days ago and have been running it here on KDE since without any issues.

Copy link
Member

@Emantor Emantor left a 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 {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
stdenv.mkDerivation rec {
mkDerivation rec {

@@ -0,0 +1,24 @@
{ stdenv, fetchFromGitHub, cmake, qtbase, qtmultimedia }:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
{ stdenv, fetchFromGitHub, cmake, qtbase, qtmultimedia }:
{ mkDerivation, lib, fetchFromGitHub, cmake, qtbase, qtmultimedia }:


nativeBuildInputs = [ cmake ];

meta = with stdenv.lib; {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
meta = with stdenv.lib; {
meta = with lib; {

buildInputs = [ qtbase qtmultimedia qtquickcontrols qtkeychain library libsecret ];

nativeBuildInputs = [ cmake qttools ];
stdenv.mkDerivation rec {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
stdenv.mkDerivation rec {
mkDerivation rec {

@colemickens
Copy link
Member Author

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.

@doronbehar
Copy link
Contributor

doronbehar commented Apr 10, 2020

I got segmentation fault after login and pressing "No" here:

NUX-selection:2020 04 10-13:21:12

But I can't reproduce... Anyway, this sounds like an upstream issue we shouldn't quarrel about.

Besides that, all looks good to me.

@peterhoeg
Copy link
Member

Should be libquotient instead of libQuotient. Otherwise good to go!

@colemickens
Copy link
Member Author

Hmm. Why should it be libquotient? It seems to be libQuotient everywhere in the source and the cmake name is capitalized as well: project(Quotient VERSION "${API_VERSION}.0" LANGUAGES CXX).

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 libturbojpeg/libjpeg_turbo.

Is there a convention here we follow or am I reading the situation wrong? Thanks!

@doronbehar
Copy link
Contributor

Yea I too think it's not that important @colemickens . The best advice for a "wait libfoo is not packaged?" is to use nix search lib foo which most of the times works. @colemickens Could you please fix the merge conflicts?

@peterhoeg
Copy link
Member

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".

@doronbehar
Copy link
Contributor

@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 libQuotient to libquotient. Don't forget the aliases.nix as well.

@worldofpeace
Copy link
Contributor

The nixpkgs manual actually mentions that attributes in all-packages.nix should be all lowercase like this.

@colemickens
Copy link
Member Author

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.

@colemickens
Copy link
Member Author

Alright, this should be closer. Thanks for the guidance.

@colemickens colemickens changed the title quaternion: update, libQuotient: init quaternion: update, libquotient: init Apr 27, 2020
@worldofpeace worldofpeace merged commit f1d1dca into NixOS:master Apr 28, 2020
@DIzFer DIzFer mentioned this pull request May 6, 2020
37 tasks
@colemickens colemickens deleted the nixpkgs-quaternion branch December 30, 2022 01:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants