Package: guix-patches;
Reported by: Andy Wingo <wingo <at> pobox.com>
Date: Mon, 24 Apr 2017 20:54:02 UTC
Severity: important
Done: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
Bug is archived. No further changes may be made.
View this message in rfc822 format
From: ludo <at> gnu.org (Ludovic Courtès) To: Andy Wingo <wingo <at> igalia.com> Cc: 26645 <at> debbugs.gnu.org Subject: bug#26645: [PATCH 1/9] guix: Add "potluck" packages. Date: Wed, 03 May 2017 22:19:34 +0200
Hi!
Finally some review for all these exciting bits! :-)
Andy Wingo <wingo <at> igalia.com> skribis:
> * guix/potluck/build-systems.scm:
> * guix/potluck/licenses.scm:
> * guix/potluck/packages.scm: New files.
> * guix/scripts/build.scm (load-package-or-derivation-from-file):
> (options->things-to-build, options->derivations): Add "potluck-package" and
> "potluck-source" to environment of file. Lower potluck packages to Guix
> packages.
[...]
> +(define-module (guix potluck build-systems)
> + #:use-module ((guix build-system) #:select (build-system?))
> + #:use-module ((gnu packages) #:select (scheme-modules))
> + #:use-module (ice-9 match)
> + #:export (build-system-by-name all-potluck-build-system-names))
> +
> +(define all-build-systems
> + (delay
> + (let* ((gbs (or (search-path %load-path "guix/build-system.scm")
> + (error "can't find (guix build-system)")))
> + (root (dirname (dirname gbs)))
> + (by-name (make-hash-table)))
> + (for-each (lambda (iface)
> + (module-for-each
> + (lambda (k var)
> + (let* ((str (symbol->string k))
> + (pos (string-contains str "-build-system"))
> + (val (variable-ref var)))
> + (when (and pos (build-system? val))
> + (let* ((head (substring str 0 pos))
> + (tail (substring str
> + (+ pos (string-length
> + "-build-system"))))
> + (name (string->symbol
> + (string-append head tail))))
> + (hashq-set! by-name name val)))))
> + iface))
> + (scheme-modules root "guix/build-system"))
> + by-name)))
What about adding a ‘lookup-build-system’ procedure in (guix
build-systems) directly that would reuse the logic from ‘fold-packages’
and co.? That would avoid repetition.
I can move the relevant bits to (guix plugins) or (guix discovery),
which should help, WDYT?
> +(define-module (guix potluck licenses)
> + #:use-module ((guix licenses) #:select (license?))
> + #:use-module (ice-9 match)
> + #:export (license-by-name all-potluck-license-names))
> +
> +(define all-licenses
> + (delay
> + (let ((iface (resolve-interface '(guix licenses)))
> + (by-name (make-hash-table)))
> + (module-for-each (lambda (k var)
> + (let ((val (variable-ref var)))
> + (when (license? val)
> + (hashq-set! by-name k val))))
> + (resolve-interface '(guix licenses)))
> + by-name)))
Likewise here.
> +(define-module (guix potluck packages)
Nice!
> +(define (potluck-package-field-location package field)
> + "Return the source code location of the definition of FIELD for PACKAGE, or
> +#f if it could not be determined."
> + (define (goto port line column)
> + (unless (and (= (port-column port) (- column 1))
> + (= (port-line port) (- line 1)))
> + (unless (eof-object? (read-char port))
> + (goto port line column))))
> +
> + (match (potluck-package-location package)
> + (($ <location> file line column)
> + (catch 'system
> + (lambda ()
> + ;; In general we want to keep relative file names for modules.
> + (with-fluids ((%file-port-name-canonicalization 'relative))
> + (call-with-input-file (search-path %load-path file)
> + (lambda (port)
> + (goto port line column)
> + (match (read port)
> + (('potluck-package inits ...)
Can we factorize it with ‘package-field-location’? In fact, it looks
like we could extract:
(define (sexp-location start-location car)
"Return the location of the sexp with the given CAR, starting from
START-LOCATION."
…)
and define both ‘package-field-location’ and
‘potluck-package-field-location’ in terms of it. Thoughts?
> +(define (lower-potluck-package pkg)
> + (validate-potluck-package pkg)
> + (let ((name (potluck-package-name pkg))
> + (version (potluck-package-version pkg))
> + (source (potluck-package-source pkg))
> + (build-system (potluck-package-build-system pkg))
> + (inputs (potluck-package-inputs pkg))
> + (native-inputs (potluck-package-native-inputs pkg))
> + (propagated-inputs (potluck-package-propagated-inputs pkg))
> + (arguments (potluck-package-arguments pkg))
> + (home-page (potluck-package-home-page pkg))
> + (synopsis (potluck-package-synopsis pkg))
> + (description (potluck-package-description pkg))
> + (license (potluck-package-license pkg)))
> + (package
> + (name name)
> + (version version)
> + (source (lower-potluck-source source))
> + (build-system (build-system-by-name build-system))
> + (inputs (lower-inputs inputs))
> + (native-inputs (lower-inputs native-inputs))
> + (propagated-inputs (lower-inputs propagated-inputs))
> + (arguments arguments)
> + (home-page home-page)
> + (synopsis synopsis)
> + (description description)
> + (license (license-by-name license)))))
Could you add a couple of tests for this?
> diff --git a/guix/scripts/build.scm b/guix/scripts/build.scm
> index 6bb1f72eb..be26f63c9 100644
> --- a/guix/scripts/build.scm
> +++ b/guix/scripts/build.scm
I’d move this part to a separate patch.
As discussed on IRC I think, I was wondering whether it would make sense
to have a ‘guix potluck build’ command instead. Normally, use
‘%standard-build-options’ and ‘set-build-options-from-command-line’ from
(guix scripts build), there should be little duplication, I think. That
would avoid entangling potluck and ‘guix build’ too much.
Could you check if that’s doable? If it turns out it’s too
inconvenient, then we can take the approach here.
Thank you!
Ludo’.
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.