[PATCH] gnu: cargo: Simplify unpacking.

  • Done
  • quality assurance status badge
Details
2 participants
  • Danny Milosavljevic
  • Ludovic Courtès
Owner
unassigned
Submitted by
Danny Milosavljevic
Severity
normal

Debbugs page

Danny Milosavljevic wrote 8 years ago
(address . guix-patches@gnu.org)(name . Danny Milosavljevic)(address . dannym@scratchpost.org)
20170319002844.19407-1-dannym@scratchpost.org
* gnu/packages/rust.scm (cargo): Simplify unpacking.
---
gnu/packages/rust.scm | 60 +++++++++++++++++++++++++++++++++------------------
1 file changed, 39 insertions(+), 21 deletions(-)

Toggle diff (96 lines)
diff --git a/gnu/packages/rust.scm b/gnu/packages/rust.scm
index 2dedef837..9a6f25c82 100644
--- a/gnu/packages/rust.scm
+++ b/gnu/packages/rust.scm
@@ -285,6 +285,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")
@@ -788,6 +790,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.
@@ -796,30 +803,40 @@ 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"))))))
+ (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)))
+ (_ #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")
+ ;(setenv "CARGO_HOME" (string-append (getcwd) "/cargohome"))
+ (call-with-output-file ".cargo/config"
(lambda (p)
(format p "
[source.crates-io]
@@ -831,7 +848,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
Ludovic Courtès wrote 8 years ago
(name . Danny Milosavljevic)(address . dannym@scratchpost.org)(address . 26166@debbugs.gnu.org)
87pogosznr.fsf@gnu.org
Hi Danny,

Danny Milosavljevic <dannym@scratchpost.org> skribis:

Toggle quote (2 lines)
> * 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.

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

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

Toggle quote (34 lines)
> (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 …)

Toggle quote (2 lines)
> + (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’.
Danny Milosavljevic wrote 8 years ago
[PATCH v2] gnu: cargo: Simplify unpacking.
(address . 26166@debbugs.gnu.org)(name . Danny Milosavljevic)(address . dannym@scratchpost.org)
20170413220402.8076-1-dannym@scratchpost.org
* 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(-)

Toggle diff (92 lines)
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
Danny Milosavljevic wrote 8 years ago
Re: bug#26166: [PATCH] gnu: cargo: Simplify unpacking.
(name . Ludovic Courtès)(address . ludo@gnu.org)(address . 26166@debbugs.gnu.org)
20170414002052.612ed7ae@scratchpost.org
Hi Ludo,

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

Toggle quote (2 lines)
> 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. :)

Toggle quote (2 lines)
> For clarity it may help to replace the ‘let’ with “internal defines”,

Done.

Toggle quote (3 lines)
> 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?).
Ludovic Courtès wrote 8 years ago
(name . Danny Milosavljevic)(address . dannym@scratchpost.org)(address . 26166@debbugs.gnu.org)
87mvbcracu.fsf@gnu.org
Hi Danny,

Danny Milosavljevic <dannym@scratchpost.org> skribis:

Toggle quote (9 lines)
> 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.

Toggle quote (26 lines)
>> 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!

Toggle quote (5 lines)
> 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’.
Danny Milosavljevic wrote 8 years ago
(no subject)
(address . control@debbugs.gnu.org)
20170507212106.7b1c20b4@scratchpost.org
close 26166
close 26541
close 26614
close 26751
close 26692
close 26731
close 26743
close 26744
?
Your comment

This issue is archived.

To comment on this conversation send an email to 26166@debbugs.gnu.org

To respond to this issue using the mumi CLI, first switch to it
mumi current 26166
Then, you may apply the latest patchset in this issue (with sign off)
mumi am -- -s
Or, compose a reply to this issue
mumi compose
Or, send patches to this issue
mumi send-email *.patch
You may also tag this issue. See list of standard tags. For example, to set the confirmed and easy tags
mumi command -t +confirmed -t +easy
Or, remove the moreinfo tag and set the help tag
mumi command -t -moreinfo -t +help