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

cryptomator: init at 1.5.13 #83434

Merged
merged 1 commit into from Mar 2, 2021
Merged

Conversation

bachp
Copy link
Member

@bachp bachp commented Mar 26, 2020

Motivation for this change

Package the cryptomator encryption utilitiy.

Closes #75684

Things done

The current version can be started from the command line and is packaged from source.
It doesn't include a desktop file to launch it. But i think this can be added later.

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

@bachp
Copy link
Member Author

bachp commented Mar 26, 2020

Looks like fuse is not properly working yet. I need to investigate some more.

@bachp
Copy link
Member Author

bachp commented Mar 26, 2020

Fuse is now found, however mounting is not possible.
The error is the following:

fusermount: /nix/store/v6l2sacryfr88yqq0pq7sia8wfgm9q31-wrapper.c:203: main: Assertion `!(st.st_mode & S_ISUID) || (st.st_uid == geteuid())' failed.

I think it might be related to #42117

@bachp
Copy link
Member Author

bachp commented Mar 26, 2020

Also related #81208

@bachp bachp marked this pull request as draft May 19, 2020 20:19
@stale
Copy link

stale bot commented Nov 16, 2020

I marked this as stale due to inactivity. → More info

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Nov 16, 2020
@bachp
Copy link
Member Author

bachp commented Nov 16, 2020

I'm still working on it.

It is still not working but I was able to compile it from source. I will push the changes soon.

@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Nov 16, 2020
@ewok-old
Copy link
Contributor

Hi @bachp !
Thank you for your great job!
I've taken the liberty and took your scripts and slightly adjusted them.
I managed to run and now I am using Cryptomator with this changes bachp/nixpkgs@cryptomator...ewok:bachp-cryptomator-1.5.10 . Hope you find it useful.

@bachp
Copy link
Member Author

bachp commented Dec 18, 2020

@ewok Awsome, I never managed to get it working. I made some progress on compiling cryptomator from source tough. I never pushed it tough.

Are you planning to submit your work to nixpkgs?

@ewok-old
Copy link
Contributor

@bachp I can submit it to nixpkgs. Could you advice how to do it correctly? Not sure that I have appropriate permissions, I am a newbie here =)

@bachp
Copy link
Member Author

bachp commented Dec 23, 2020

@ewok You don't need any special permissions. You just have to open a pull request. When you open a new pull request it will include a template that should give you some hints on what to look out for.

And don't be afraid to open a PR, I think contributing to nixpks is very aprachable and people are very friendly and help you out should you miss somthing.

@bachp
Copy link
Member Author

bachp commented Dec 23, 2020

@ewok If you prefer I could also include your changes in this MR

@bachp bachp force-pushed the cryptomator branch 3 times, most recently from 4ebee09 to 94aef5f Compare December 23, 2020 23:22
@bachp bachp changed the title cryptomator: init at 1.4.15 cryptomator: init at 1.5.11 Dec 23, 2020
@bachp bachp marked this pull request as ready for review December 23, 2020 23:25
@ivankovnatsky
Copy link
Contributor

ivankovnatsky commented Jan 4, 2021

@bachp, @ewok this is great, thank you for your work.

nixpkgs-review pr 83434

1 package built:
cryptomator

i was able to build and run cryptomator (added my vault and browsed my files no problem):

used fuse backend.

pkgs/tools/security/cryptomator/default.nix Show resolved Hide resolved
pkgs/tools/security/cryptomator/default.nix Outdated Show resolved Hide resolved
pkgs/tools/security/cryptomator/default.nix Show resolved Hide resolved
pkgs/tools/security/cryptomator/default.nix Outdated Show resolved Hide resolved
pkgs/tools/security/cryptomator/default.nix Outdated Show resolved Hide resolved
@ivankovnatsky
Copy link
Contributor

ivankovnatsky commented Jan 4, 2021

tried to mount with webdav:

Caused by: org.cryptomator.frontend.webdav.mount.Mounter$CommandFailedException: No mounting strategy found.
        at org.cryptomator.frontend.webdav.mount.FallbackMounter.mount(FallbackMounter.java:15)
        at org.cryptomator.frontend.webdav.servlet.WebDavServletController.mount(WebDavServletController.java:102)
        at org.cryptomator.common.vaults.WebDavVolume.mount(WebDavVolume.java:66)
        ... 12 common frames omitted

forgive my ignorance, do i understand it right that we don't package in AppImage format due to fuse issues? i see that arch package it in AppImage: https://aur.archlinux.org/cgit/aur.git/tree/PKGBUILD?h=cryptomator, no problem.

@bachp
Copy link
Member Author

bachp commented Jan 4, 2021

@ivankovnatsky I started of with packaging the AppImage but, as you already said, fuse didn't work inside the wrapper. I belive the reason is #42117. I also tried to manually extract the AppImage but I couldn't get it working either.

But I think packaging from source is preferable anyway and thanks to the work of @ewok it's now working.

I didn't test Webdav to be honest as I prefer fuse. Do you consider this a blocker?

@ivankovnatsky
Copy link
Contributor

ivankovnatsky commented Jan 5, 2021

@bachp

can you please squash your changes/suggestions together.

I started of with packaging the AppImage but, as you already said, fuse didn't work inside the wrapper. I belive the reason is #42117. I also tried to manually extract the AppImage but I couldn't get it working either.

But I think packaging from source is preferable anyway and thanks to the work of @ewok it's now working.

yeah, i myself got into these troubles when was trying to use slock display locker, it would not work unless used with: https://github.com/NixOS/nixpkgs/blob/master/nixos/modules/programs/slock.nix#L24.

potentially we could do the same, i guess, but i'm here occasional contributor, we might need to consult members of the community.

in case of packaging the AppImage format we don't need to think about support like webdav and so, as it should be provided as is, hopefully :-)

I didn't test Webdav to be honest as I prefer fuse. Do you consider this a blocker?
i myself don't care much, as long as my files are mounted in readable form, but i've used cryptomator on both windows and mac and it uses webdav by default there.

i mean some users might want that, but not really sure. and as cryptomator is a gui app (please correct me if not), i think we need to support full features in the interface.

@Lassulus
Copy link
Member

hi,
can you please import the graphics from somewhere else? they are rather big and nixpkgs should not contain any asset files to keep the size from growing needlessly.

@NobbZ
Copy link
Contributor

NobbZ commented Feb 16, 2021

What do you mean by "from somewhere else"?

Despite their source, wont they end up in $out and therefore be in the binary cache anyway? Or am I somehow misunderstanding you?

@Lassulus
Copy link
Member

Lassulus commented Feb 16, 2021

yes, they will end up on the binary cache, but every nixos/nix system has a full checkout of nixpkgs lying around. even if they never use this package they would end up with some asset files they can't get rid of.

from somewhere else, maybe via fetchurl? then we would just have a link inside the nix expression. Question is, if there is a stable url available for these images. In the best case they would be part of the source itself, so maybe this should be fixed in their source repo?

@matthiasbeyer
Copy link
Contributor

I agree with @Lassulus - This stuff does not belong into nixpkgs! Adding images to nixpkgs results in thousand of nix installations being spammed with unnecessary crap! Don't do this!

@NobbZ
Copy link
Contributor

NobbZ commented Feb 16, 2021

@Lassulus Okay, I think I understand now. I haven't seen that there were pictures part of the commit(s).

@Mic92
Copy link
Member

Mic92 commented Feb 16, 2021

I agree with @Lassulus - This stuff does not belong into nixpkgs! Adding images to nixpkgs results in thousand of nix installations being spammed with unnecessary crap! Don't do this!

language.

@matthiasbeyer
Copy link
Contributor

language.

I did not target any contributors or person here! If someone felt offended anyways, I apologize.

Copy link
Member

@primeos primeos left a comment

Choose a reason for hiding this comment

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

Requesting changes for ./org.cryptomator.Cryptomator.{svg,png} but I've also noted a few other more optional nitpicks that I found along the way.

Anyway, props for managing to package this! Seems like there where quite some challenges.

pkgs/tools/security/cryptomator/default.nix Outdated Show resolved Hide resolved
pkgs/tools/security/cryptomator/default.nix Outdated Show resolved Hide resolved
pkgs/tools/security/cryptomator/default.nix Outdated Show resolved Hide resolved
pkgs/tools/security/cryptomator/default.nix Outdated Show resolved Hide resolved
pkgs/tools/security/cryptomator/default.nix Outdated Show resolved Hide resolved
pkgs/tools/security/cryptomator/default.nix Outdated Show resolved Hide resolved
pkgs/tools/security/cryptomator/default.nix Outdated Show resolved Hide resolved
pkgs/tools/security/cryptomator/default.nix Outdated Show resolved Hide resolved
@SuperSandro2000
Copy link
Member

This is a semi-automatic executed nixpkgs-review with nixpkgs-review-checks extension. It is checked by a human on a best effort basis and does not build all packages (e.g. lumo, tensorflow or pytorch).
If you have any questions or problems please reach out to SuperSandro2000 on IRC.

Result of nixpkgs-review pr 83434 run on x86_64-linux 1

1 package built:
  • cryptomator

The following issues got detected with the above build packages.
Please fix at least the ones listed with your changed packages:

cryptomator:

An avoidable string conversion got detected: rev = "${version}"; rev = "${version}";
Please do not convert variables to a string without modifying them but use them directly instead.

gpl3 is a deprecated license, check if project uses gpl3Plus or gpl3Only and change meta.license accordingly.

Near pkgs/tools/security/cryptomator/default.nix:91:5:

   |
91 |     license = licenses.gpl3;
   |     ^

See: https://github.com/jtojnar/nixpkgs-hammering/blob/master/explanations/unclear-gpl.md
buildPhase should probably contain runHook preBuild and runHook postBuild.

Near pkgs/tools/security/cryptomator/default.nix:56:3:

   |
56 |   buildPhase = ''
   |   ^

See: https://github.com/jtojnar/nixpkgs-hammering/blob/master/explanations/missing-phase-hooks.md
installPhase should probably contain runHook preInstall and runHook postInstall.

Near pkgs/tools/security/cryptomator/default.nix:61:3:

   |
61 |   installPhase = ''
   |   ^

See: https://github.com/jtojnar/nixpkgs-hammering/blob/master/explanations/missing-phase-hooks.md
Unused argument: fetchurl.
Near pkgs/tools/security/cryptomator/default.nix:1:16:

  |
1 | { lib, stdenv, fetchurl, fetchFromGitHub
  |                ^

Unused argument: makeDesktopItem.
Near pkgs/tools/security/cryptomator/default.nix:3:3:

  |
3 | , makeDesktopItem
  |   ^

pkgs/tools/security/cryptomator/default.nix Outdated Show resolved Hide resolved
pkgs/tools/security/cryptomator/default.nix Outdated Show resolved Hide resolved
pkgs/tools/security/cryptomator/default.nix Outdated Show resolved Hide resolved
pkgs/tools/security/cryptomator/default.nix Outdated Show resolved Hide resolved
@bachp
Copy link
Member Author

bachp commented Feb 18, 2021

@SuperSandro2000 Suggestions comments applied

@primeos primeos dismissed their stale review February 18, 2021 17:06

All requested changes where resolved. The diff LGTM now.

@SuperSandro2000
Copy link
Member

This is a semi-automatic executed nixpkgs-review with nixpkgs-review-checks extension. It is checked by a human on a best effort basis and does not build all packages (e.g. lumo, tensorflow or pytorch).
If you have any questions or problems please reach out to SuperSandro2000 on IRC.

Result of nixpkgs-review pr 83434 run on x86_64-linux 1

1 package built:
  • cryptomator

The following issues got detected with the above build packages.
Please fix at least the ones listed with your changed packages:

cryptomator:

gpl3 is a deprecated license, check if project uses gpl3Plus or gpl3Only and change meta.license accordingly.

Near pkgs/tools/security/cryptomator/default.nix:87:5:

   |
87 |     license = licenses.gpl3;
   |     ^

See: https://github.com/jtojnar/nixpkgs-hammering/blob/master/explanations/unclear-gpl.md
buildPhase should probably contain runHook preBuild and runHook postBuild.

Near pkgs/tools/security/cryptomator/default.nix:53:3:

   |
53 |   buildPhase = ''
   |   ^

See: https://github.com/jtojnar/nixpkgs-hammering/blob/master/explanations/missing-phase-hooks.md
installPhase should probably contain runHook preInstall and runHook postInstall.

Near pkgs/tools/security/cryptomator/default.nix:58:3:

   |
58 |   installPhase = ''
   |   ^

See: https://github.com/jtojnar/nixpkgs-hammering/blob/master/explanations/missing-phase-hooks.md

Co-authored-by: Artur Taranchiev <ewok@ewok.ru>
@bachp
Copy link
Member Author

bachp commented Feb 25, 2021

Updated to 1.5.13

@bachp bachp changed the title cryptomator: init at 1.5.12 cryptomator: init at 1.5.13 Feb 25, 2021
@SuperSandro2000 SuperSandro2000 merged commit c62b506 into NixOS:master Mar 2, 2021
@bachp bachp deleted the cryptomator branch March 2, 2021 16:50
@ivankovnatsky
Copy link
Contributor

@bachp great work! :-)

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.

Packaging request: Cryptomator