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

psi-plus: init at 0.16.572.639 #23597

Merged
merged 3 commits into from Mar 18, 2017
Merged

psi-plus: init at 0.16.572.639 #23597

merged 3 commits into from Mar 18, 2017

Conversation

orivej
Copy link
Contributor

@orivej orivej commented Mar 7, 2017

Motivation for this change

Psi+ is a great Jabber client, successor of Psi.

Things done
  • Tested using sandboxing
    (nix.useSandbox on NixOS,
    or option build-use-sandbox in nix.conf
    on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • Linux
  • 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/)
  • Fits CONTRIBUTING.md.

ndowens
ndowens suggested changes Mar 8, 2017
View reviewed changes
pkgs/applications/networking/instant-messengers/psi-plus/default.nix Outdated
enableParallelBuilding = true;

meta = {
description = "Psi+, an XMPP (Jabber) client";
Copy link
Contributor

Choose a reason for hiding this comment

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

Would say that description should be like XMPP (Jabber) client, Psi+ is in a way the name of the program, in which guidelines say shouldn't start with.

Copy link
Contributor

@ndowens ndowens Mar 8, 2017 •
edited

Choose a reason for hiding this comment

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

Is "which" really needed?
pkgconfig should go in nativeBuildInputs as it is a build dependency instead of a dependency that is needed to run.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the review, I updated the pull request. which is needed, otherwise configure fails with You don't seem to have 'make' or 'gmake' in your PATH..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It turned out that psi-plus has two build systems, and after switching to cmake to build plugins, which and pkgconfig became unnecessary.

pkgs/applications/networking/instant-messengers/psi-plus/default.nix Outdated

meta = {
description = "Psi+, an XMPP (Jabber) client";
maintainers = with stdenv.lib.maintainers; [ orivej ];
Copy link
Contributor

@ndowens ndowens Mar 8, 2017 •
edited

Choose a reason for hiding this comment

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

Instead of writing stdenv.lib inside meta several times, could do meta = with stdenv.lib; { so there is no need of using it on the rest of the content.

@orivej orivej force-pushed the psi-plus branch 3 times, most recently from b031b5c to 2825ff4 Compare March 9, 2017 03:11
@Mic92
Copy link
Member

Mic92 commented Mar 9, 2017

I get a build error, when compiling this package:

Scanning dependencies of target whiteboarding
[ 24%] Building CXX object src/whiteboarding/CMakeFiles/whiteboarding.dir/moc_wbdlg.cpp.o
In file included from /tmp/nix-build-psi-plus-0.16.572.639.drv-0/psi-plus-snapshots-0.16.572.639-src/src/psiaccount.h:39:0,
                 from /tmp/nix-build-psi-plus-0.16.572.639.drv-0/psi-plus-snapshots-0.16.572.639-src/build/src/whiteboarding/../../../src/whiteboarding/../sxe/sxesession.h:33,
                 from /tmp/nix-build-psi-plus-0.16.572.639.drv-0/psi-plus-snapshots-0.16.572.639-src/build/src/whiteboarding/../../../src/whiteboarding/wbwidget.h:25,
                 from /tmp/nix-build-psi-plus-0.16.572.639.drv-0/psi-plus-snapshots-0.16.572.639-src/build/src/whiteboarding/../../../src/whiteboarding/wbdlg.h:42,
                 from /tmp/nix-build-psi-plus-0.16.572.639.drv-0/psi-plus-snapshots-0.16.572.639-src/build/src/whiteboarding/moc_wbdlg.cpp:9:
/tmp/nix-build-psi-plus-0.16.572.639.drv-0/psi-plus-snapshots-0.16.572.639-src/src/mucjoindlg.h:26:24: fatal error: ui_mucjoin.h: No such file or directory
compilation terminated.
make[2]: *** [src/whiteboarding/CMakeFiles/whiteboarding.dir/build.make:83: src/whiteboarding/CMakeFiles/whiteboarding.dir/moc_wbdlg.cpp.o] Error 1
make[1]: *** [CMakeFiles/Makefile2:1265: src/whiteboarding/CMakeFiles/whiteboarding.dir/all] Error 2
make: *** [Makefile:128: all] Error 2

@orivej
Copy link
Contributor Author

orivej commented Mar 9, 2017

It would help if you could post the complete build log. Mine is at https://gist.github.com/orivej/b7e783378bd44f2b5d72271c0a4c9837

@orivej
Copy link
Contributor Author

orivej commented Mar 10, 2017 •
edited

@Mic92 Travis build on amd64 has passed, and I have not seen anything like your error while packaging psi-plus, and I have been building on a multicore server so it is not likely to be an issue with parallelized compilation. Can you give more details? This may have been not the first error in your build log.

@Mic92
Copy link
Member

Mic92 commented Mar 11, 2017 •
edited

@orivej if you rebase it against master:

$ git fetch origin master # origin should point to github.com/Nixos/nixpkgs
$ git rebase --onto c3c69535aa9f18f611950655d2e8ffc82521943d HEAD~3 HEAD

You hopefully get the following build error:

https://dl.thalheim.io/TeqOuLNtYDwsHKxk-ry9FA/build.log

@orivej orivej force-pushed the psi-plus branch 3 times, most recently from b4b198c to dc6f5cc Compare March 18, 2017 21:15
@orivej
Copy link
Contributor Author

orivej commented Mar 18, 2017 •
edited

It was an issue with parallel building. It has been fixed upstream, and I updated the package version in the pull request.

@7c6f434c
Copy link
Member

I wonder if this version still crashes when connecting to servers with untrusted certificates…

@7c6f434c 7c6f434c merged commit 081557a into NixOS:master Mar 18, 2017
@orivej
Copy link
Contributor Author

orivej commented Mar 18, 2017

For me it does not, let me know if you see a crash.

@orivej orivej deleted the psi-plus branch March 18, 2017 22:53
@7c6f434c
Copy link
Member

right now Qt 5.8 cannot be built…

@orivej
Copy link
Contributor Author

orivej commented Mar 19, 2017

#24036 fixes Qt build

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ndowens ndowens ndowens approved these changes

Assignees
No one assigned
Labels
8.has: package (new)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@orivej @Mic92 @7c6f434c @ndowens @joachifm