bug#26166: [PATCH] gnu: cargo: Simplify unpacking.

bug#26166: [PATCH] gnu: cargo: Simplify unpacking.

[Ludovic Courtès]

Hi Danny,

Danny Milosavljevic <dannym@scratchpost.org> skribis:

> * gnu/packages/rust.scm (cargo): Simplify unpacking.

One minor issue: Please describe the changes in terms of code: add
#:modules, modify ‘unpack-submodule-sources’ phase such that this and
that, etc.

>  gnu/packages/rust.scm | 60 +++++++++++++++++++++++++++++++++------------------
>  1 file changed, 39 insertions(+), 21 deletions(-)

It’s not immediately obvious that it’s a simplification.  ;-)

>           (delete 'patch-usr-bin-file)
>           (add-after 'unpack 'unpack-submodule-sources
>             (lambda* (#:key inputs #:allow-other-keys)
> -             (let ((unpack (lambda (source target)
> -                             (mkdir-p target)
> -                             (with-directory-excursion target
> -                               (zero? (system* "tar" "xf"
> -                                               source
> -                                               "--strip-components=1"))))))
> +             (let* ((unpack
> +                      (lambda (source target)
> +                        (mkdir-p target)
> +                        (with-directory-excursion target
> +                          (zero? (system* "tar" "xf"
> +                                          source
> +                                          "--strip-components=1")))))
> +                    (touch
> +                      (lambda (file-name)
> +                        (call-with-output-file file-name (const #t))))
> +                    (install-rust-library
> +                      (lambda (entry)
> +                        (match entry
> +                          ((name . src)
> +                           (if (string-prefix? "rust-" name)
> +                             (let* ((rust-length (string-length "rust-"))
> +                                    (rust-name (string-drop name
> +                                                            rust-length))
> +                                    (rsrc (string-append "vendor/"
> +                                                         rust-name))
> +                                    (unpack-status (unpack src rsrc)))
> +                               (touch (string-append rsrc "/.cargo-ok"))
> +                               (generate-checksums rsrc src)
> +                               unpack-status)))

For clarity it may help to replace the ‘let’ with “internal defines”,
like this:

  (lambda* …
    (define (unpack source target)
      …)
    (define (touch file)
      …)
    (define (install-rust-library entry)
      …)

    body …)

> +               (mkdir ".cargo")
> +               ;(setenv "CARGO_HOME" (string-append (getcwd) "/cargohome"))
                  ^
Leftover?

I don’t fully understand this file, but if it sounds good to you, we
should apply it.  OK to send an updated patch?

Thank you!

Ludo’.

bug#26166: [PATCH v2] gnu: cargo: Simplify unpacking.

[Danny Milosavljevic]

* gnu/packages/rust.scm (cargo): Simplify unpacking.
[arguments]<:modules>: Add (srfi srfi-1).
[arguments]<:phases>: Adapt 'unpack-submodule-sources' phase to more clearly
seperate the tasks it does.  Add helper procedures 'unpack', 'touch',
'install-rust-library'.
[arguments]<:phases>: Rename 'set-cargo-home' to 'set-environment-up' and
make it use official cargo directories.
[arguments]<:phases>: Remove 'configure' phase.
---
 gnu/packages/rust.scm | 56 ++++++++++++++++++++++++++++++++-------------------
 1 file changed, 35 insertions(+), 21 deletions(-)

diff --git a/gnu/packages/rust.scm b/gnu/packages/rust.scm
index a9710ba4d..1217ec71f 100644
--- a/gnu/packages/rust.scm
+++ b/gnu/packages/rust.scm
@@ -301,6 +301,8 @@ safety and thread safety guarantees.")
     ;; Dual licensed.
     (license (list license:asl2.0 license:expat))))
 
+;; This tries very hard not to get into a cyclic dependency like this:
+;;   cargo <- cargo-build-system <- cargo.
 (define-public cargo
   (package
     (name "cargo")
@@ -825,6 +827,11 @@ safety and thread safety guarantees.")
     (arguments
      `(#:cargo ,cargo-bootstrap
        #:tests? #f ; FIXME
+       #:modules
+       ((ice-9 match)
+        (srfi srfi-1) ; 'every
+        (guix build utils)
+        (guix build cargo-build-system))
        #:phases
        (modify-phases %standard-phases
          ;; Avoid cargo complaining about missmatched checksums.
@@ -833,30 +840,36 @@ safety and thread safety guarantees.")
          (delete 'patch-usr-bin-file)
          (add-after 'unpack 'unpack-submodule-sources
            (lambda* (#:key inputs #:allow-other-keys)
-             (let ((unpack (lambda (source target)
-                             (mkdir-p target)
-                             (with-directory-excursion target
-                               (zero? (system* "tar" "xf"
-                                               source
-                                               "--strip-components=1"))))))
+             (define (unpack source target)
+               (mkdir-p target)
+               (with-directory-excursion target
+                 (zero? (system* "tar" "xf"
+                                 source
+                                 "--strip-components=1"))))
+             (define (touch file-name)
+               (call-with-output-file file-name (const #t)))
+             (define (install-rust-library entry)
+               (match entry
+                 ((name . src)
+                  (if (string-prefix? "rust-" name)
+                    (let* ((rust-length (string-length "rust-"))
+                           (rust-name (string-drop name
+                                                   rust-length))
+                           (rsrc (string-append "vendor/"
+                                                rust-name))
+                           (unpack-status (unpack src rsrc)))
+                      (touch (string-append rsrc "/.cargo-ok"))
+                      (generate-checksums rsrc src)
+                      unpack-status)))
+                 (_ #t)))
                (mkdir "vendor")
-               (for-each (lambda (p)
-                           (let ((name (car p)))
-                             (if (string-prefix? "rust-" name)
-                               (let ((rsrc (string-append "vendor/"
-                                                           (string-drop name
-                                                                        (string-length "rust-")))))
-                                 (unpack (assoc-ref inputs name) rsrc)
-                                 (system* "touch" (string-append rsrc "/.cargo-ok"))
-                                 (generate-checksums rsrc (assoc-ref inputs name)))))) inputs))))
-         ;; Set CARGO_HOME to use the vendored dependencies.
-         (add-after 'unpack 'set-cargo-home
+               (every install-rust-library inputs)))
+         (add-after 'unpack 'set-environment-up
            (lambda* (#:key inputs #:allow-other-keys)
              (let* ((gcc (assoc-ref inputs "gcc"))
                     (cc (string-append gcc "/bin/gcc")))
-               (mkdir "cargohome")
-               (setenv "CARGO_HOME" (string-append (getcwd) "/cargohome"))
-               (call-with-output-file "cargohome/config"
+               (mkdir ".cargo")
+               (call-with-output-file ".cargo/config"
                  (lambda (p)
                    (format p "
 [source.crates-io]
@@ -868,7 +881,8 @@ directory = 'vendor'
 ")))
                (setenv "CMAKE_C_COMPILER" cc)
                (setenv "CC" cc))
-             #t)))))
+             #t))
+         (delete 'configure))))
     (home-page "https://github.com/rust-lang/cargo")
     (synopsis "Build tool and package manager for Rust")
     (description "Cargo is a tool that allows Rust projects to declare their

bug#26166: [PATCH] gnu: cargo: Simplify unpacking.

[Danny Milosavljevic]

Hi Ludo,

On Fri, 07 Apr 2017 22:58:32 +0200
ludo@gnu.org (Ludovic Courtès) wrote:

> It’s not immediately obvious that it’s a simplification.  ;-)

Yeah well.  Simple doesn't mean shorter.  I think it's clearer to understand what it's doing when there a procedures that have names suggesting what it's for. :)

> For clarity it may help to replace the ‘let’ with “internal defines”,

Done.

> I don’t fully understand this file, but if it sounds good to you, we
> should apply it.

Yeah, Rust stuff is definitely not straightforward.

What this does is it sets up dependencies that are required to build cargo - just like cargo-vendor (an extension of cargo) would have set them up.

This avoids another bootstrapping problem (we would have to have cargo-vendor binaries).  But cargo-vendor is an entirely avoidable dependency because the format of their package metadata is stable (and very minimal).

The format of the metadata is stable because cargo-vendor is the official way to bundle libraries with your custom project - we just bundle them on-the-fly to avoid bundling them in the cargo distribution file (like David's version did before) and previously having to distribute our own custom version of cargo.

cargo-vendor is a way that any developer on the world can use to bundle stuff for his Rust project *and check it into his git repository*.  That means that metadata is on git repos Mozilla doesn't control - which means the format has to be stable (or at least backward-compatible).

Now we replace cargo-vendor entirely.  Both cargo-build-system and this cargo package do cargo-vendors job in Guile (that's what install-rust-library does; now I wonder whether I should call it 'bundle-rust-library' instead.  WDYT?).

bug#26166: [PATCH] gnu: cargo: Simplify unpacking.

[Ludovic Courtès]

Hi Danny,

Danny Milosavljevic <dannym@scratchpost.org> skribis:

> On Fri, 07 Apr 2017 22:58:32 +0200 ludo@gnu.org (Ludovic Courtès)
> wrote:
>
>> It’s not immediately obvious that it’s a simplification.  ;-)
>
> Yeah well.  Simple doesn't mean shorter.  I think it's clearer to
> understand what it's doing when there a procedures that have names
> suggesting what it's for. :)

Right.

>> I don’t fully understand this file, but if it sounds good to you, we
>> should apply it.
>
> Yeah, Rust stuff is definitely not straightforward.
>
> What this does is it sets up dependencies that are required to build
> cargo - just like cargo-vendor (an extension of cargo) would have set
> them up.
>
> This avoids another bootstrapping problem (we would have to have
> cargo-vendor binaries).  But cargo-vendor is an entirely avoidable
> dependency because the format of their package metadata is stable (and
> very minimal).
>
> The format of the metadata is stable because cargo-vendor is the
> official way to bundle libraries with your custom project - we just
> bundle them on-the-fly to avoid bundling them in the cargo
> distribution file (like David's version did before) and previously
> having to distribute our own custom version of cargo.
>
> cargo-vendor is a way that any developer on the world can use to
> bundle stuff for his Rust project *and check it into his git
> repository*.  That means that metadata is on git repos Mozilla doesn't
> control - which means the format has to be stable (or at least
> backward-compatible).

OK, thanks for the explanation.  If you can think of pointers we can add
in comments to make it easier for future hackers to find out about this,
feel free to add them.

In the meantime, we shouldn’t block this patch any longer, so go ahead!

> Now we replace cargo-vendor entirely.  Both cargo-build-system and
> this cargo package do cargo-vendors job in Guile (that's what
> install-rust-library does; now I wonder whether I should call it
> 'bundle-rust-library' instead.  WDYT?).

Dunno, ‘install-rust-library’ sounds good to me.

Thanks,
Ludo’.