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

nextcloud-client: miscellaneous fixes #29256

Merged
merged 4 commits into from Sep 14, 2017
Merged

nextcloud-client: miscellaneous fixes #29256

merged 4 commits into from Sep 14, 2017

Conversation

eqyiel
Copy link
Contributor

@eqyiel eqyiel commented Sep 12, 2017

Motivation for this change

I've been maintaining a version of this outside of nixpkgs for a while and since this is a thing now I would like to contribute the difference to this expression.

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 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 nox --run "nox-review wip"

  • Tested execution of all binary files (usually in ./result/bin/)

  • Fits CONTRIBUTING.md.

  • enabled parallel building

  • use existing cmake* attrs rather than manipulating cmake flags in preConfigure

  • use inotify for more efficient monitoring of file changes

  • provide gnome-keyring integration when built with withGnomeKeying = true;

I'm not sure if adding gnome_keyring to LD_LIBRARY_PATH is the correct way to go about this, but it's what worked for me.

@caugner thanks for your work on this package.

@eqyiel eqyiel changed the title Nextcloud client fixes nextcloud-client: miscellaneous fixes Sep 12, 2017
bjornfor
bjornfor requested changes Sep 12, 2017
View reviewed changes
Copy link
Contributor

@bjornfor bjornfor left a comment

Choose a reason for hiding this comment

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

"nextcloud-client: add optional inotify dependency" -- it doesn't look optional to me. It looks like it will break if "inotify-tools = null". I'm fine with inotify being hard dependency on Linux, but wondering if this expression currently works on Darwin and that adding inotify will break that?

@eqyiel
Copy link
Contributor Author

eqyiel commented Sep 12, 2017

My bad, I'll make it so that it checks stdenv.isLinux - what I intended to express is that nextcloud-client tries to find inotify at build time and uses it if it's available.

Happily I'm at my workplace right now where I have access to a Mac to build on, and the build fails with

make[2]: Entering directory '/private/var/folders/jt/qgrx8lp10ylbdlnb93t54r6c0000gn/T/nix-build-alsa-lib-1.1.4.1.drv-0/alsa-lib-1.1.4.1/src/control'
  CC       cards.lo
In file included from cards.c:35:
In file included from ./control_local.h:22:
../../include/local.h:47:2: error: Header defining endianness not defined
#error Header defining endianness not defined
 ^
1 error generated.
make[2]: *** [Makefile:334: cards.lo] Error 1
make[2]: Leaving directory '/private/var/folders/jt/qgrx8lp10ylbdlnb93t54r6c0000gn/T/nix-build-alsa-lib-1.1.4.1.drv-0/alsa-lib-1.1.4.1/src/control'
make[1]: *** [Makefile:478: all-recursive] Error 1
make[1]: Leaving directory '/private/var/folders/jt/qgrx8lp10ylbdlnb93t54r6c0000gn/T/nix-build-alsa-lib-1.1.4.1.drv-0/alsa-lib-1.1.4.1/src'
make: *** [Makefile:338: all-recursive] Error 1
builder for ‘/nix/store/9djjg7idiqp7awyx6v3568fkykacrs3l-alsa-lib-1.1.4.1.drv’ failed with exit code 2
fetching path ‘/nix/store/3ip043cbchgj6m4hjfiwn0qw87jdcaz0-qtbase-5.6.2-dev’...
cannot build derivation ‘/nix/store/2chdyipixw3ncqisy3316w98vrwaj9m5-qtmultimedia-5.6.2.drv’: 1 dependencies couldn't be built
cannot build derivation ‘/nix/store/z6yl0n1s54ny3zfqz660kfx14w8sn3l0-qtlocation-5.6.2.drv’: 1 dependencies couldn't be built
cannot build derivation ‘/nix/store/24axc64rwn91h8yk8hv8b6vlg2r7jgqz-qtwebkit-5.6.2.drv’: 1 dependencies couldn't be built
killing process 7940
cannot build derivation ‘/nix/store/hn2r0a5186ygw93bxzwh7w74y5v0ik9i-nextcloud-client-2.3.2.drv’: 1 dependencies couldn't be built
error: build of ‘/nix/store/hn2r0a5186ygw93bxzwh7w74y5v0ik9i-nextcloud-client-2.3.2.drv’ failed
nix-build -A nextcloud-client -I nixpkgs=.  5.87s user 4.06s system 54% cpu 18.223 total

Which is a dependency of qtmultimedia and was not introduced by this commit.

I'm certain this can build on Darwin, but I think that's out of scope for this PR.

@eqyiel
nextcloud-client: use cmakeDir and cmakeFlags attrs
ebb6488
@eqyiel
nextcloud-client: add inotify dependency when building on linux
e02f936 
This allows for more efficient change detection.
@eqyiel
nextcloud-client: enable parallel building
4ce18c8 
So that the client can build faster.
@eqyiel
nextcloud-client: provide optional gnome-keyring integration
e828dcb 
`qtkeyring` can use `gnome-keyring`, but it needs some help to find it.

I have not enabled this by default because not everyone who uses this will want
to pull in GNOME dependencies.
@eqyiel
Copy link
Contributor Author

eqyiel commented Sep 13, 2017

Rebased!

@bjornfor bjornfor merged commit e828dcb into NixOS:master Sep 14, 2017
@bjornfor
Copy link
Contributor

Tested locally. Pushed to master (e828dcb and parents) and release-17.09 (e61f1a2 and parents). Thanks!

@eqyiel
Copy link
Contributor Author

eqyiel commented Sep 14, 2017

@bjornfor thank you!

@eqyiel eqyiel deleted the nextcloud-client-fixes branch September 14, 2017 23:31
@caugner
Copy link
Contributor

caugner commented Sep 15, 2017

Thank you @eqy and @bjornfor!

In general, it would make sense to merge owncloud-client and nextcloud-client in the sense that the nextcloud-client config could inherit from the owncloud-client. Unfortunately, I don't have the Nix experience to accomplish this.

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

@bjornfor bjornfor bjornfor requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@eqyiel @bjornfor @caugner