On Mon, Dec 06, 2021 at 11:17:47PM +0100, Ludovic Courtès wrote:
> Efraim Flashner <efraim@flashner.co.il> skribis:
> 
> > On December 6, 2021 4:37:12 PM UTC, "Ludovic Courtès" <ludo@gnu.org> wrote:
> 
> [...]
> 
> >>Thinking out loud… would it work to change:
> >>
> >>  (arguments '(#:cargo-inputs X #:cargo-development-inputs Y))
> >>
> >>to:
> >>
> >>  (native-inputs (map package-source Y))
> >>  (inputs (map package-source X))
> >>
> >>?
> 
> [...]
> 
> > Then we lose the transitive package sources, which is how we ended up where we are today.
> 
> True.
> 
> With the minimal changes to (guix build-system cargo) below, one can use
> either the current style or pass “development inputs” as ‘native-inputs’
> and other dependencies as ‘inputs’.  Source transitivity is preserved
> but you can write packages the normal way.
> 
> I modified some of the dependencies of librsvg to use
> native-inputs/inputs and you can see when applying this part of the
> patch that the librsvg derivation is unchanged.  Good thing is that
> ‘guix graph’ and ‘guix refresh -l’ work for these packages.
> 
> Is this a direction we want to take?

I like the way it works out, and has Guix do the magic to give us the
crates in the graph. On the other hand I tried changing the cargo-inputs
from librsvg to regular inputs, and after 2.5 minutes of trying to run
`guix show librsvg` I still wasn't seeing the dependencies and my RAM
usage was still increasing. Also gnu/packages/gnome.scm didn't fail to
compile, so there was no notice of the loop.

I changed some more packages which are transitive inputs of
rust-encoding@0.2 and didn't see any slowdown. I was worried that this
would affect a future use of `guix shell -D rust-app` not pulling in any
of the crates but it still seems to work. I tried with rust-encoding@0.2
and got the crates for the packages I expected (only the ones I changed).

(ins)efraim@3900XT ~/workspace/guix-core-updates$ ./pre-inst-env guix shell -D rust-encoding@0.2
(ins)efraim@3900XT ~/workspace/guix-core-updates [env]$ ls $GUIX_ENVIRONMENT/share/cargo/registry/ | col
cc-1.0.66.crate
compiler_builtins-0.1.26.crate
encoding-index-japanese-1.20141219.5.crate
encoding-index-korean-1.20141219.5.crate
encoding-index-simpchinese-1.20141219.5.crate
encoding-index-singlebyte-1.20141219.5.crate
encoding_index_tests-0.1.4.crate
encoding-index-tradchinese-1.20141219.5.crate
getopts-0.2.21.crate
log-0.3.9.crate
rustc-std-workspace-core-1.0.0.crate
rustc-std-workspace-std-1.0.1.crate
unicode-width-0.1.9.crate

So to summarize, between your diff to treat inputs built using
cargo-build-system as cargo-inputs and my changes to save previous
crates for the next input we reach a place where we can start to change
the crates over to use inputs and native-inputs instead of cargo-inputs
and cargo-development-inputs without needing to flip everything at once.

So I'd go with it's good, but I'm not sure it directly works to fix the
problem we're having with librsvg.

> If so, we can have ‘guix style’ automate transformations.
> 
> Thanks,
> Ludo’.
> 

I've added some inline comments in the diff (and removed a bunch of
lines)

> diff --git a/guix/build-system/cargo.scm b/guix/build-system/cargo.scm
> index 60c35eed07..b2d97beb2f 100644
> --- a/guix/build-system/cargo.scm
> +++ b/guix/build-system/cargo.scm
> @@ -125,17 +125,21 @@ (define builder
>                      #:target #f
>                      #:guile-for-build guile))
>  
> +(define (cargo-input? input)
> +  (match input
> +    ((label (? package? p))
> +     (eq? cargo-build-system (package-build-system p)))
> +    (_ #f)))
> +

I would've sorted based on the name starting with 'rust-'.

I can't think of an example quickly where it would happen, but take
librsvg, which we currently build with cargo-build-system. If we need it
as an input for pango (or some other library) and we also build that
with the cargo-build-system then we'll just get the rust inputs, not the
actual library.

Can we check the arguments field for `#:install-source? #f` and use it
as a regular {propagated-,native-,}input in that case? Then we could
have cbindgen and rust-cbindgen, depending on if we needed the binary or
the sources.

>  (define (package-cargo-inputs p)
> -  (apply
> -    (lambda* (#:key (cargo-inputs '()) #:allow-other-keys)
> -      cargo-inputs)
> -    (package-arguments p)))
> +  (match (member #:cargo-inputs (package-arguments p))
> +    (#f (filter cargo-input? (package-inputs p)))
> +    ((_ inputs . _) inputs)))
>  
>  (define (package-cargo-development-inputs p)
> -  (apply
> -    (lambda* (#:key (cargo-development-inputs '()) #:allow-other-keys)
> -      cargo-development-inputs)
> -    (package-arguments p)))
> +  (match (member #:cargo-development-inputs (package-arguments p))
> +    (#f (filter cargo-input? (package-native-inputs p)))
> +    ((_ inputs . _) inputs)))

I see we don't get rid of #:cargo-inputs or #:cargo-development-inputs.
So even if applying the style change to all the crates causes circular
dependency problems we can fall back to the current method. I ran into
problems once I hit all the rust-bindgen crates.

>  
>  (define (crate-closure inputs)
>    "Return the closure of INPUTS when considering the 'cargo-inputs' and
> @@ -235,8 +239,8 @@ (define (expand-crate-sources cargo-inputs cargo-development-inputs)
>  (define* (lower name
>                  #:key source inputs native-inputs outputs system target
>                  (rust (default-rust))
> -                (cargo-inputs '())
> -                (cargo-development-inputs '())
> +                (cargo-inputs (filter cargo-input? inputs))
> +                (cargo-development-inputs (filter cargo-input? native-inputs))

I tried commenting the cargo-development-inputs out, but it only caused
problems for me when trying to compile rust-encoding@0.2.

>                  #:allow-other-keys
>                  #:rest arguments)
>    "Return a bag for NAME."
> @@ -260,7 +264,9 @@ (define private-keywords
>           (build-inputs `(("cargo" ,rust "cargo")
>                           ("rustc" ,rust)
>                           ,@(expand-crate-sources cargo-inputs cargo-development-inputs)
> -                         ,@native-inputs))
> +                         ,@(if (eq? native-inputs cargo-development-inputs)
> +                               '()
> +                               native-inputs)))
>           (outputs outputs)
>           (build cargo-build)
>           (arguments (strip-keyword-arguments private-keywords arguments)))))
> diff --git a/guix/packages.scm b/guix/packages.scm
> index b3c5a00011..275cc3675c 100644
> --- a/guix/packages.scm
> +++ b/guix/packages.scm
> @@ -660,7 +660,8 @@ (define (deprecated-package old-name p)
>      (name old-name)
>      (properties `((superseded . ,p)))))
>  
> -(define (package-field-location package field)
> +(define* (package-field-location package field
> +                                 #:key (value-location? #t))
>    "Return the source code location of the definition of FIELD for PACKAGE, or
>  #f if it could not be determined."
>    (match (package-location package)
> @@ -678,7 +679,10 @@ (define (package-field-location package field)
>                     (let ((field (assoc field inits)))
>                       (match field
>                         ((_ value)
> -                        (let ((loc (and=> (source-properties value)
> +                        (let ((loc (and=> (source-properties
> +                                           (if value-location?
> +                                               value
> +                                               field))
>                                            source-properties->location)))
>                            (and loc
>                                 ;; Preserve the original file name, which may be a
> diff --git a/guix/scripts/style.scm b/guix/scripts/style.scm
> index 86a46f693c..dccc20d880 100644
> --- a/guix/scripts/style.scm
> +++ b/guix/scripts/style.scm
> @@ -29,6 +29,7 @@
>  
>  (define-module (guix scripts style)
>    #:autoload   (gnu packages) (specification->package fold-packages)
> +  #:autoload   (guix build-system cargo) (cargo-build-system)
>    #:use-module (guix scripts)
>    #:use-module ((guix scripts build) #:select (%standard-build-options))
>    #:use-module (guix combinators)
> @@ -212,6 +213,21 @@ (define (object->string* obj indent)
>        (pretty-print-with-comments port obj
>                                    #:indent indent))))
>  
> +(define (object-list->string lst indent)
> +  (call-with-output-string
> +    (lambda (port)
> +      (let loop ((lst lst))
> +        (match lst
> +          ((obj)
> +           (pretty-print-with-comments port obj
> +                                       #:indent indent))
> +          ((obj rest ...)
> +           (pretty-print-with-comments port obj
> +                                       #:indent indent)
> +           (newline port)
> +           (display (make-string indent #\space) port)
> +           (loop rest)))))))
> +
>  
>  ;;;
>  ;;; Simplifying input expressions.
> @@ -441,6 +457,49 @@ (define matches?
>              (list package-inputs package-native-inputs
>                    package-propagated-inputs)))
>  
> +
> +;;;
> +;;; Crates, Cargo, Rust, and all that.
> +;;;
> +
> +(define* (rewrite-cargo-inputs package
> +                               #:key (policy 'silent)
> +                               (edit-expression edit-expression))
> +  (when (eq? (package-build-system package) cargo-build-system)
> +    (match (package-field-location package 'arguments
> +                                   #:value-location? #f)
> +      (#f #f)
> +      (location
> +       (let* ((indent (location-column location)))
> +         (edit-expression
> +          (pk 'loc (location->source-properties location))
> +          (lambda (str)
> +            (define arguments
> +              (call-with-input-string (pk 'str str) read-with-comments))
> +
> +            (match arguments
> +              (('arguments ('quasiquote lst))
> +               (let ((inputs (match (member #:cargo-inputs lst)
> +                               (#f '())
> +                               ((_ inputs . _) inputs)))
> +                     (native (match (member #:cargo-development-inputs lst)
> +                               (#f '())
> +                               ((_ inputs . _) inputs)))
> +                     (rest (strip-keyword-arguments
> +                            '(#:cargo-inputs #:cargo-development-inputs)
> +                            lst)))
> +                 (object-list->string
> +                  `(,@(if (null? rest)
> +                          '()
> +                          `((arguments ,(list 'quasiquote rest))))
> +                    ,@(if (null? native)
> +                          '()
> +                          `((native-inputs ,(list 'quasiquote native))))
> +                    ,@(if (null? inputs)
> +                          '()
> +                          `((inputs ,(list 'quasiquote inputs)))))
> +                  indent)))))))))))
> +
>  (define (package-location<? p1 p2)
>    "Return true if P1's location is \"before\" P2's."
>    (let ((loc1 (package-location p1))
> @@ -536,7 +595,7 @@ (define (parse-options)
>                         edit-expression))
>           (policy   (assoc-ref opts 'input-simplification-policy)))
>      (for-each (lambda (package)
> -                (simplify-package-inputs package #:policy policy
> +                (rewrite-cargo-inputs package #:policy policy
>                                           #:edit-expression edit))
>                ;; Sort package by source code location so that we start editing
>                ;; files from the bottom and going upward.  That way, the


-- 
Efraim Flashner   <efraim@flashner.co.il>   רנשלפ םירפא
GPG key = A28B F40C 3E55 1372 662D  14F7 41AA E7DC CA3D 8351
Confidentiality cannot be guaranteed on emails sent or received unencrypted