make-system-tarball: allow alternate compression methods #37288

cleverca22 · 2018-03-18T00:32:20Z

Motivation for this change

sometimes an xz based tarball cant be used, and sometimes you may want to compress in parallel with pixz or pigz

Things done

added support for gzip, lzma, bzip2, parallel xz/gz
tested with nix-build nixos/release.nix -A containerTarball.x86_64-linux

Tested using sandboxing (nix.useSandbox on NixOS, or option build-use-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/)
Fits CONTRIBUTING.md.

the stats i gathered during testing:


xz:
real: 5m, user: 4m53s, size: 118mb

pixz:
real: 1m20, user: 5m26s, size: 129m

gzip:
real: 52sec, user: 39sec, size: 209mb

pigz:
real: 26sec, user: 41sec, size: 209mb

bzip2:
real: 1m39s, user: 1m18s, size: 180mb

lzma:
real: 5m3s, user: 4m56s, size: 118mb

disassembler

I like the options for building release tarballs. I built locally since I'm not sure if borg can handle arguments and everything worked as expected.

disassembler · 2018-03-18T00:41:51Z

nixos/lib/make-system-tarball.sh

@@ -54,8 +53,8 @@ mkdir -p $out/tarball

 rm env-vars

-tar --sort=name --mtime='@1' --owner=0 --group=0 --numeric-owner -cvJf $out/tarball/$fileName.tar.xz * $extraArgs
+time tar --sort=name --mtime='@1' --owner=0 --group=0 --numeric-owner -c * $extraArgs | $compression $compressorFlags > $out/tarball/$fileName.tar.${extension}


did you intend to leave time in here or was it just for testing?

a bit of both, with the -v gone its a bit too silent, and it could be handy to know how long just the compression phase took, since half the time is taken up by a cp

lheckemann

I like it! Maybe a no-compression option would be nice too.

lheckemann · 2018-03-18T11:30:08Z

nixos/lib/make-system-tarball.nix

+    gzip = gzip;
+    pigz = pigz;
+    bzip2 = bzip2;
+    lzma = lzma;


could just use inherit xz pixz gzip pigz bzip2 lzma;

cleverca22 · 2018-03-18T14:04:29Z

@grahamc

2018-03-18 11:03:15 < clever> gchristensen: how does this look to you?, and would there be any way for ofborg to check nixos/release.nix and see what has changed? https://github.com/NixOS/nixpkgs/pull/37288

grahamc · 2018-03-19T14:07:02Z

nixos/lib/make-system-tarball.nix


-  inherit fileName pathsFromGraph extraArgs extraCommands;
+  inherit fileName pathsFromGraph extraArgs extraCommands compressorFlags;
+  compression = if (compression == "none") then "cat" else compression;


Maybe move this in to the inputMap so there is no special case.

inputMap is a list of derivations to have in buildInputs, but this one is a string, what command to pipe the tar file thru, i would need a 3rd mapping

Perhaps instead:

{ compression = { none = { inputs = [ ]; extension = ""; command = "cat"; }; pigz = { inputs = [ pigz ]; extension = ".pigz"; command = "pigz"; }; }; }

?

yeah, that can work

grahamc · 2018-03-19T14:07:14Z

nixos/lib/make-system-tarball.sh

@@ -1,5 +1,4 @@
 source $stdenv/setup
-set -x


it was overly verbose, and i thought that this is typically only kept in when debugging things

grahamc · 2018-03-19T14:08:57Z

nixos/lib/make-system-tarball.nix

+  inputMap = {
+    inherit xz pixz gzip pigz bzip2 lzma;
+    none = null;
+  };


I don't love that this isn't extendable by the caller, but perhaps it is not necessary?

i could also just add some more arguments, that default to looking things up in the map, so you can bypass the map easily?

edolstra · 2018-03-19T14:56:07Z

To be honest, this seems like over-engineering. If you really need a different compression method, you can easily keep that in a local git tree.

cleverca22 · 2018-03-19T14:57:12Z

mainly wanted to avoid duplicating the code

grahamc · 2018-03-19T14:57:45Z

Perhaps, then, the best way would be to allow overriding but not build-in the different compression algos.

cleverca22 · 2018-03-19T14:58:37Z

that can work, just move everything to arguments, and remove the mappings

grahamc · 2018-04-03T12:57:58Z

nixos/lib/make-system-tarball.nix

+  # extra inputs, like the compressor to use
+, extraInputs ? [ pixz ]
+  # flags passed to the compressor
+, compressorFlags ? ""


Maybe drop compressorFlags and just pass yourcmd -abc as the command, instead of as a separate parameter. This simplifies the calling code and we don't do anything special at all with the arguments anyway. Beyond this, which is a minor nit, this LGTM.

lheckemann · 2018-04-04T15:14:56Z

nixos/lib/make-system-tarball.nix

+  # Extension for the compressed tarball
+, compressionExtension ? ".xz"
+  # extra inputs, like the compressor to use
+, extraInputs ? [ pixz ]


Rather than this, compressCommand could just be ${pixz}/bin/pixz?

GrahamcOfBorg added 6.topic: nixos 8.has: documentation 8.has: module (update) labels Mar 18, 2018

disassembler approved these changes Mar 18, 2018

View reviewed changes

GrahamcOfBorg added 10.rebuild-darwin: 0 10.rebuild-linux: 0 labels Mar 18, 2018

lheckemann approved these changes Mar 18, 2018

View reviewed changes

cleverca22 force-pushed the improve-make-tarball branch from 4e905fe to 14cf486 Compare March 18, 2018 12:31

grahamc reviewed Mar 19, 2018

View reviewed changes

cleverca22 force-pushed the improve-make-tarball branch from 14cf486 to 31b8de9 Compare March 24, 2018 16:11

grahamc reviewed Apr 3, 2018

View reviewed changes

make-system-tarball: allow alternate compression methods

3c9e579

cleverca22 force-pushed the improve-make-tarball branch from 31b8de9 to 3c9e579 Compare April 3, 2018 14:31

grahamc merged commit 9b30d48 into NixOS:master Apr 4, 2018

lheckemann reviewed Apr 4, 2018

View reviewed changes

cleverca22 deleted the improve-make-tarball branch April 4, 2018 21:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

make-system-tarball: allow alternate compression methods #37288

make-system-tarball: allow alternate compression methods #37288

cleverca22 commented Mar 18, 2018 •

edited

disassembler left a comment

disassembler Mar 18, 2018

cleverca22 Mar 18, 2018

lheckemann left a comment

lheckemann Mar 18, 2018

cleverca22 commented Mar 18, 2018

grahamc Mar 19, 2018

cleverca22 Mar 19, 2018

grahamc Mar 19, 2018 •

edited

cleverca22 Mar 19, 2018

grahamc Mar 19, 2018

cleverca22 Mar 19, 2018

grahamc Mar 19, 2018

cleverca22 Mar 19, 2018

edolstra commented Mar 19, 2018

cleverca22 commented Mar 19, 2018

grahamc commented Mar 19, 2018

cleverca22 commented Mar 19, 2018

grahamc Apr 3, 2018

lheckemann Apr 4, 2018

make-system-tarball: allow alternate compression methods #37288

make-system-tarball: allow alternate compression methods #37288

Conversation

cleverca22 commented Mar 18, 2018 • edited

Motivation for this change

Things done

disassembler left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lheckemann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cleverca22 commented Mar 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grahamc Mar 19, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edolstra commented Mar 19, 2018

cleverca22 commented Mar 19, 2018

grahamc commented Mar 19, 2018

cleverca22 commented Mar 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cleverca22 commented Mar 18, 2018 •

edited

grahamc Mar 19, 2018 •

edited