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

rsync: 3.1.3 -> 3.2.3; add additional dependencies #99061

Merged
merged 1 commit into from Oct 2, 2020

Conversation

@andrew-d
Copy link
Contributor

@andrew-d andrew-d commented Sep 29, 2020

Motivation for this change

I wanted support for zstd compression in rsync; I added LZ4, OpenSSL, and xxHash support as well.

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) (33936936 ➡️ 40755760)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
@doronbehar
Copy link
Contributor

@doronbehar doronbehar commented Sep 29, 2020

Please target staging branch, and fix merge conflict.

@andrew-d
Copy link
Contributor Author

@andrew-d andrew-d commented Sep 30, 2020

@GrahamcOfBorg build rsync

@andrew-d
Copy link
Contributor Author

@andrew-d andrew-d commented Sep 30, 2020

@doronbehar - Done!

@doronbehar
Copy link
Contributor

@doronbehar doronbehar commented Sep 30, 2020

@GrahamcOfBorg build rsync

^ To test the darwin build.

It's funny - ofborg labeled the PR with "0: has cleanup" and I'm a bit surprised that aarch64 is not supported, at least now - I don't know if rsync was available to aarch64 before this change...

@ehmry
Copy link
Member

@ehmry ehmry commented Sep 30, 2020

It's funny - ofborg labeled the PR with "0: has cleanup" and I'm a bit surprised that aarch64 is not supported, at least now - I don't know if rsync was available to aarch64 before this change...

I'm using rsync on aarch64 without any problems.

I'm a bit skeptical of adding an OpenSSL dependency, but otherwise it looks good to me.

@ehmry
ehmry approved these changes Sep 30, 2020
@doronbehar
Copy link
Contributor

@doronbehar doronbehar commented Sep 30, 2020

It's funny - ofborg labeled the PR with "0: has cleanup" and I'm a bit surprised that aarch64 is not supported, at least now - I don't know if rsync was available to aarch64 before this change...

I'm using rsync on aarch64 without any problems.

@ehmry could you verify that on aarch64 you can build rsync with this PR merged? I tried to trace the new dependencies for either of them or their deps to be marked marked as broken or unsupported for aarch64 but I couldn't find any such trace. Also, you don't have by any chance allowing unsupported systems on that aarch64 machine?

@ehmry
Copy link
Member

@ehmry ehmry commented Oct 2, 2020

@doronbehar I don't have enough bandwith to rebuild on staging but I can evaluate the build.

@doronbehar
Copy link
Contributor

@doronbehar doronbehar commented Oct 2, 2020

@doronbehar I don't have enough bandwith to rebuild on staging but I can evaluate the build.

Yes please. Ofborg says it won't evaluate on aarch64, and I only wish to know why. This should do it:

git clone https://github.com/andrew-d/nixpkgs --branch andrew/rsync-3.2
cd nixpkgs
nix-build --dry-run --show-trace -A rsync
@doronbehar
Copy link
Contributor

@doronbehar doronbehar commented Oct 2, 2020

@ehmry forget it. The aarch64 failing ofborg evaluation is unrelated and all PRs are affected:

#99316

Add zstd, lz4, openssl and xxHash as optional dependencies, to support
more compression formats.
@doronbehar doronbehar force-pushed the andrew-d:andrew/rsync-3.2 branch from f9cf583 to 79c01c8 Oct 2, 2020
@doronbehar
Copy link
Contributor

@doronbehar doronbehar commented Oct 2, 2020

Did nothing besides changing the commit message a bit. Merging without waiting for CI, as it was green before.

@doronbehar doronbehar merged commit 820b251 into NixOS:staging Oct 2, 2020
2 of 3 checks passed
2 of 3 checks passed
tests tests
Details
action
Details
Wait for ofborg This failed status will be cleared when ofborg finishes eval.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.