[bug#36841,v3] build/cargo-build-system: Patch cargo checksums.
diff mbox series

Message ID 20190730104658.GC21431@E2140
State New
Headers show
Series
  • [bug#36841,v3] build/cargo-build-system: Patch cargo checksums.
Related show

Commit Message

Efraim Flashner July 30, 2019, 10:46 a.m. UTC
On Tue, Jul 30, 2019 at 11:17:57AM +0300, Efraim Flashner wrote:
> On Mon, Jul 29, 2019 at 06:44:31PM -0700, Ivan Petkov wrote:
> > Hi Efraim,
> > 
> > > On Jul 29, 2019, at 12:04 PM, Efraim Flashner <efraim@flashner.co.il> wrote:
> > > 
> > > +;; After patching the 'patch-generated-file-shebangs phase any vendored crates
> > > +;; will have a mismatch on their checksum.
> > > +(define* (patch-cargo-checksums #:key
> > > +                                (vendor-dir "guix-vendor")
> > > +                                #:allow-other-keys)
> > 
> > [snip]
> > 
> > > +    (replace 'install install)
> > > +    (add-after 'patch-generated-file-shebangs 'patch-cargo-checksums patch-cargo-checksums)))
> > 
> > I can’t quite remember the order the phases run in off the top of my head. Would it be possible to
> > make the configure/checksum generation phase run after shebang-patching (or ensure the patching
> > happens first)? It would avoid having to checksum all the files twice that way…
> 
> The 'configure phase could be renamed the plop-vendored-crates-into-place
> phase. It actually can't come after the 'patch-generated-file-shebangs
> phase since then there won't be any vendored crates to patch.
> 
> If we remove the generate-checksums call from 'configure then there
> won't be a .cargo-checksum.json to remove and regenerate during the
> 'patch-cargo-checksums phase, so I've changed that to search for
> "Cargo.toml$" and not delete it. Not as robust as "for each top-level
> directory in the 'vendor-dir'", but should be good enough.
> 
This one I'm pretty happy with. The checksums are only generated twice
when there's a Cargo.lock file present and I've factored out the
function to generate all the checksums. When that's moved to (guix build
cargo-utils) it can be used by the rust compilers and icecat.

Comments

Ivan Petkov Aug. 1, 2019, 3 a.m. UTC | #1
Hi Efraim,

> On Jul 30, 2019, at 3:46 AM, Efraim Flashner <efraim@flashner.co.il> wrote:
> 
> This one I'm pretty happy with. The checksums are only generated twice
> when there's a Cargo.lock file present and I've factored out the
> function to generate all the checksums. When that's moved to (guix build
> cargo-utils) it can be used by the rust compilers and icecat.

Overall the patch makes sense to me!

However, I am curious what are some of the situations in which you’re encountering
a Cargo.lock file? In a system like guix which maintains all dependencies immutably
and consistently, the Cargo.lock file is virtually useless (in fact it *could* be harmful
if an application is released with a Cargo.lock file pinning to a particular vulnerable
dependency which needs to be updated, requiring patching of the Cargo.lock file).

I’d be willing to go as far as suggest we unconditionally delete any Cargo.lock file
in source tarballs and let cargo generate its own replacement using the vendor
directory we have supplied. (Imports from crates.io <http://crates.io/> also never include a Cargo.lock
file, so this may only pertain if we’re performing a direct source import…)

—Ivan
Efraim Flashner Aug. 1, 2019, 11:15 a.m. UTC | #2
On Wed, Jul 31, 2019 at 08:00:00PM -0700, Ivan Petkov wrote:
> Hi Efraim,
> 
> > On Jul 30, 2019, at 3:46 AM, Efraim Flashner <efraim@flashner.co.il> wrote:
> > 
> > This one I'm pretty happy with. The checksums are only generated twice
> > when there's a Cargo.lock file present and I've factored out the
> > function to generate all the checksums. When that's moved to (guix build
> > cargo-utils) it can be used by the rust compilers and icecat.
> 
> Overall the patch makes sense to me!
> 
> However, I am curious what are some of the situations in which you’re encountering
> a Cargo.lock file? In a system like guix which maintains all dependencies immutably
> and consistently, the Cargo.lock file is virtually useless (in fact it *could* be harmful
> if an application is released with a Cargo.lock file pinning to a particular vulnerable
> dependency which needs to be updated, requiring patching of the Cargo.lock file).

One is the package that I'm actually targeting, https://github.com/chfi/rust-qtlreaper/ ,
and three of the others are rust-regex and rust-compiler-builtins and
rust-env-logger. All three of them I got from $(guix import crate foo).
`guix import crate env-logger`, for example, returns this:
https://static.crates.io/crates/env_logger/env_logger-0.6.2.crate

> 
> I’d be willing to go as far as suggest we unconditionally delete any Cargo.lock file
> in source tarballs and let cargo generate its own replacement using the vendor
> directory we have supplied. (Imports from crates.io <http://crates.io/> also never include a Cargo.lock
> file, so this may only pertain if we’re performing a direct source import…)

This is basically what my 'update-cargo-lock phase does. Otherwise we
end up packaging arbitrary versions of crates to satisfy whatever
version they were using when they last updated their Cargo.lock.

> 
> —Ivan
Efraim Flashner Aug. 4, 2019, 8:57 a.m. UTC | #3
Pushed with an update to the documentation

Patch
diff mbox series

diff --git a/guix/build/cargo-build-system.scm b/guix/build/cargo-build-system.scm
index f38de16cf7..7d363a18a5 100644
--- a/guix/build/cargo-build-system.scm
+++ b/guix/build/cargo-build-system.scm
@@ -2,6 +2,7 @@ 
 ;;; Copyright © 2016 David Craven <david@craven.ch>
 ;;; Copyright © 2017 Mathieu Othacehe <m.othacehe@gmail.com>
 ;;; Copyright © 2019 Ivan Petkov <ivanppetkov@gmail.com>
+;;; Copyright © 2019 Efraim Flashner <efraim@flashner.co.il>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -39,6 +40,21 @@ 
 ;;
 ;; Code:
 
+;; TODO: Move this to (guix build cargo-utils). Will cause a full rebuild
+;; of all rust compilers.
+
+(define (generate-all-checksums dir-name)
+  (for-each
+    (lambda (filename)
+      (let* ((dir (dirname filename))
+             (checksum-file (string-append dir "/.cargo-checksum.json")))
+        (when (file-exists? checksum-file) (delete-file checksum-file))
+        (display (string-append
+                   "patch-cargo-checksums: generate-checksums for "
+                   dir "\n"))
+        (generate-checksums dir)))
+    (find-files dir-name "Cargo.toml$")))
+
 (define (manifest-targets)
   "Extract all targets from the Cargo.toml manifest"
   (let* ((port (open-input-pipe "cargo read-manifest"))
@@ -94,8 +110,7 @@  Cargo.toml file present at its root."
               ;; so that we can generate any cargo checksums.
               ;; The --strip-components argument is needed to prevent creating
               ;; an extra directory within `crate-dir`.
-              (invoke "tar" "xvf" path "-C" crate-dir "--strip-components" "1")
-              (generate-checksums crate-dir)))))
+              (invoke "tar" "xvf" path "-C" crate-dir "--strip-components" "1")))))
     inputs)
 
   ;; Configure cargo to actually use this new directory.
@@ -121,6 +136,31 @@  directory = '" port)
   (setenv "CC" (string-append (assoc-ref inputs "gcc") "/bin/gcc"))
   #t)
 
+;; The Cargo.lock file tells the build system which crates are required for
+;; building and hardcodes their version and checksum.  In order to build with
+;; the inputs we provide, we need to recreate the file with our inputs.
+(define* (update-cargo-lock #:key
+                            (vendor-dir "guix-vendor")
+                            #:allow-other-keys)
+  "Regenerate the Cargo.lock file with the current build inputs."
+  (when (file-exists? "Cargo.lock")
+    (begin
+      ;; Unfortunately we can't generate a Cargo.lock file until the checksums
+      ;; are generated, so we have an extra round of generate-all-checksums here.
+      (generate-all-checksums vendor-dir)
+      (delete-file "Cargo.lock")
+      (invoke "cargo" "generate-lockfile")))
+  #t)
+
+;; After the 'patch-generated-file-shebangs phase any vendored crates who have
+;; their shebangs patched will have a mismatch on their checksum.
+(define* (patch-cargo-checksums #:key
+                                (vendor-dir "guix-vendor")
+                                #:allow-other-keys)
+  "Patch the checksums of the vendored crates after patching their shebangs."
+  (generate-all-checksums vendor-dir)
+  #t)
+
 (define* (build #:key
                 skip-build?
                 (cargo-build-flags '("--release"))
@@ -162,7 +202,9 @@  directory = '" port)
     (replace 'configure configure)
     (replace 'build build)
     (replace 'check check)
-    (replace 'install install)))
+    (replace 'install install)
+    (add-after 'configure 'update-cargo-lock update-cargo-lock)
+    (add-after 'patch-generated-file-shebangs 'patch-cargo-checksums patch-cargo-checksums)))
 
 (define* (cargo-build #:key inputs (phases %standard-phases)
                       #:allow-other-keys #:rest args)