[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[bug#30572] [PATCH 6/7] system: Add "guix system docker-image" command.
From: |
Ludovic Courtès |
Subject: |
[bug#30572] [PATCH 6/7] system: Add "guix system docker-image" command. |
Date: |
Wed, 21 Mar 2018 21:50:00 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/25.3 (gnu/linux) |
Hi Chris,
Chris Marusich <address@hidden> skribis:
> address@hidden (Ludovic Courtès) writes:
>
>>> + (define json
>>> + ;; Pick the guile-json package that corresponds to the Guile used to
>>> build
>>> + ;; derivations.
>>> + (if (string-prefix? "2.0" (package-version (default-guile)))
>>> + guile2.0-json
>>> + guile-json))
>>
>> I think we can use ‘guile-json’ unconditionally here.
>
> Good point. Nobody using this new code will be using Guile 2.0,
Specifically, we know that derivations are built with ‘guile-final’,
which is now 2.2.
> so we can just use guile-json unconditionally. Does that mean we can
> also clean up the same conditional statement from other places in Guix
> code now?
Yes, indeed!
>>> + (mkdir root-directory)
>>> + (initialize root-directory)
>>> + (build-docker-image
>>> + (string-append "/xchg/" #$name) ;; The output file.
>>> + (cons* root-directory
>>> + (call-with-input-file (string-append "/xchg/"
>>> #$graph)
>>> + read-reference-graph))
>>> + #$os-drv
>>> + #:compressor '(#+(file-append gzip "/bin/gzip") "-9n")
>>> + #:creation-time (make-time time-utc 0 1)
>>> + #:transformations `((,root-directory -> "")))))))
>>
>> Am I right that the whole point of passing several file names to
>> ‘build-docker-image’ is that here we don’t need to copy the whole store
>> to ‘root-directory’, right?
>
> The primary reason why I made this change was because it was the
> simplest way I could find to re-use the existing code. The fact that we
> copy less is a nice secondary effect, but it is not the primary reason
> why I structured it this way. There might be a simpler way to
> accomplish this, but this way works, which I think is a good start. I
> would like to commit this as-is for now. If we can figure out a simpler
> way to implement the same logic, I'd be all for it, but it seems tricky.
>
> The original role of the "PATH" argument was surprising (for example, it
> was not actually used for adding any paths to the final Docker image!).
> In addition, the original code assumed that all the paths to add (loaded
> from the "CLOSURE" graph file argument - not from PATH!) are store
> paths, which is not true when creating a GuixSD Docker image (unless we
> try to copy everything created by root-partition-initializer back into
> the store). So, some of the paths I need to add are store paths, and
> some of them are paths to special files outside the store, like device
> files. Maybe we can copy all of this (including device files and socket
> files) into a single directory in the store (or outside of the store).
> I don't know. If it's possible, I agree it would be a nice improvement.
> But I tried various methods, and my latest patch was the simplest method
> I found that worked. I would definitely be happy if we could simplify
> it even more, but I also want to move forward with this patch series.
I see, thanks for taking the time to explain!
I’m always wary about adding too many options in an interface, but like
you wrote, it may be unavoidable here.
> Is it OK to commit this as-is (with just the guile-json change you
> suggested above)?
Yes, sure; sorry for the extra delay. We can always adjust later if we
have a better idea.
Thanks a lot for working on this!
Ludo’.
- [bug#30572] [PATCH 7/7] tests: Add tests for "guix system disk-image" et al., (continued)
- [bug#30572] [PATCH 2/7] tests: Add tests for "guix pack"., Chris Marusich, 2018/03/15
- [bug#30572] [PATCH 2/7] tests: Add tests for "guix pack"., Danny Milosavljevic, 2018/03/16
- [bug#30572] [PATCH 2/7] tests: Add tests for "guix pack"., Ludovic Courtès, 2018/03/17
- [bug#30572] [PATCH 2/7] tests: Add tests for "guix pack"., Chris Marusich, 2018/03/21
- [bug#30572] [PATCH 2/7] tests: Add tests for "guix pack"., Chris Marusich, 2018/03/21
- [bug#30572] [PATCH 2/7] tests: Add tests for "guix pack"., Chris Marusich, 2018/03/22
- [bug#30572] [PATCH 2/7] tests: Add tests for "guix pack"., Ludovic Courtès, 2018/03/22
- bug#30572: [PATCH 2/7] tests: Add tests for "guix pack"., Chris Marusich, 2018/03/23
- [bug#30572] [PATCH 2/7] tests: Add tests for "guix pack"., Ludovic Courtès, 2018/03/24