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

dirvish: init at 1.2.1 #55488

Merged
merged 4 commits into from Feb 24, 2019
Merged

dirvish: init at 1.2.1 #55488

merged 4 commits into from Feb 24, 2019

Conversation

winpat
Copy link
Member

@winpat winpat commented Feb 9, 2019

Motivation for this change

Dirvish is a battle proof wrapper around rsync.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • 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 nox --run "nox-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)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@nixos-discourse
Copy link

This pull request has been mentioned on Nix community. There might be relevant details there:

https://discourse.nixos.org/t/perl-runtime-dependency-issue-in-new-nix-package/2090/1

@thefloweringash
Copy link
Member

I packaged this for my backup server, but never got around to upstreaming it. Now that I re-read it, I find some bugs, but maybe it would still be useful to compare? https://gist.github.com/thefloweringash/1f44500cc22116558382efb1555d7dff

@winpat
Copy link
Member Author

winpat commented Feb 9, 2019

@thefloweringash Cool! I will have a look at your gist tomorrow in detail.

@winpat
Copy link
Member Author

winpat commented Feb 10, 2019

@thefloweringash This is my first time working with Nix and I'm learning quite a bit from your solution. Thanks for that!

  1. Is it ok with you if I reuse large parts of your dirvish.nix and module.nix?
  2. If so, since you already did the heavy lifting would you like to be mentioned in the meta.maintainers attribute?
  3. You also mentioned, that you've seen some bugs while re-reading it. Would you mind to elaborate on that (I would be more than happy to fix them)?

@thefloweringash
Copy link
Member

@winpat

  1. Absolutely! Thank you for up-streaming this.
  2. I don't mind much either way. I contribute where I can but haven't taken maintainership of anything yet.
  3. Things I noticed wrong with my version:
    • executables and manpages include strings with spaces and rely on shell word splitting. Should be [ "a" "b" ], not [ "a b" ].
    • rsync is an unused buildInput. It's runtime only, and added via an explicit wrapper.
    • cat a followed by cat b might as well just be cat a b
    • buildPhase creates $out/bin, which it doesn't use, then installPhase also creates it.

@winpat
Copy link
Member Author

winpat commented Feb 10, 2019

I've fixed and tested the mentioned points. I will submit the dirvish service options in a separate PR due to time constraints.

@winpat winpat changed the title (WIP) dirvish: init at 1.2.1 dirvish: init at 1.2.1 Feb 10, 2019
@xeji xeji merged commit 631bc2c into NixOS:master Feb 24, 2019
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

6 participants