Re: [PATCH v2] gnu: Add dub-build-system.

[Ludovic Courtès]

Hi Danny,

Danny Milosavljevic <address@hidden> skribis:

> * guix/build-system/dub.scm: New file.
> * guix/build/dub-build-system.scm: New file.
> * Makefile.am (MODULES): Add them.

Nice work!

Do you have experience using it on real DUB packages?  IOW, how complete
is it?  :-)

For the final patch, could you add a short description under “Build
Systems” in guix.texi?

> +(define* (dub-build store name inputs
> +                      #:key
> +                      (tests? #t)
> +                      (test-target #f)
> +                      (configure-flags #f) ; XXX unused

You can remove keyword parameters that don’t make sense like this one.

> +(define-module (guix build dub-build-system)
> +  #:use-module ((guix build gnu-build-system) #:prefix gnu:)
> +  #:use-module (guix build syscalls)
> +  #:use-module (guix build utils)
> +  #:use-module (ice-9 popen)
> +  #:use-module (ice-9 rdelim)
> +  #:use-module (ice-9 ftw)
> +  #:use-module (ice-9 format)
> +  #:use-module (ice-9 match)
> +  #:use-module (srfi srfi-1)
> +  #:use-module (srfi srfi-26)
> +  #:export (%standard-phases
> +            dub-build))
> +
> +;; Commentary:
> +;;
> +;; Builder-side code of the standard Rust package build procedure.

s/Rust/DUB/  :-)

Maybe write “DUB, the build tool for D” (or similar) to clarify.

> +;; FIXME: Needs to be parsed from url not package name.
> +(define (package-name->d-package-name name)
> +  "Return the package name of NAME."

“Return the DUB package name corresponding to NAME, a Guix package
name.” (Correct?)

> +(define* (configure #:key inputs #:allow-other-keys)
> +  "Replace argo.toml [dependencies] section with guix inputs."

Maybe “Add INPUTS to the 'dependencies' section of 'argo.toml'.”?

> +  (system* "chmod" "+w" ".")

Rather: (chmod "." #o755).


> +  (mkdir "vendor")

“vendor”?

> +  (for-each
> +    (match-lambda
> +      ((name . path)
> +       (let* ((d-package (package-name->d-package-name name))
> +              (d-basename (basename path)))
> +         (when (and d-package path)
> +           (match (string-split (basename path) #\-)
> +             ((_ ... version)
> +              (symlink (string-append path "/lib/dub/" d-basename) 
> (string-append "vendor/" d-basename))))))))
> +    inputs)

Could you add a comment above explaining why this needs to be done?

Please keep lines below 80 chars.  :-)

> +  ;(setenv "CC" (string-append (assoc-ref inputs "gcc") "/bin/gcc"))

Remove.

> +  (let* ((dir (mkdtemp! "/tmp/dub.XXXXXX")))
> +    (setenv "HOME" dir)

If HOME is relied on, please add a comment explaining why.

> +(define* (build #:key (dub-build-flags '())
> +                #:allow-other-keys)
> +  "Build a given DUB package."
> +  (if (or (zero? (system* "grep" "-q" "sourceLibrary" "package.json"))
> +          (zero? (system* "grep" "-q" "sourceLibrary" "dub.sdl")) ; note: 
> format is different!
> +          (zero? (system* "grep" "-q" "sourceLibrary" "dub.json")))
> +    #t

Would be best to avoid calling out to ‘grep’.  At worst you can do:

  (define (grep string file)
    (string-contains (call-with-input-file file get-string-all)
                     string))

(These files are probably small so it should be good enough.)

> +    (let ((status (zero? (apply system* `("dub" "build" 
> ,@dub-build-flags)))))
> +      (system* "dub" "run") ; might fail for "targetType": "library"
> +      status)))

Should be (and (zero? (apply system* "dub" "build" dub-build-flags))
               (zero? (apply system* "dub" "run")))

Or is it important to ignore the exit status of “dub run”?

> +(define* (check #:key tests? #:allow-other-keys)
> +  (if tests?
> +    (zero? (system* "dub" "test"))
> +    #t))

To be sure, could you pass the file through etc/indent-code.el?

> +(define* (install #:key inputs outputs #:allow-other-keys)
> +  "Install a given DUB package."
> +  (let* ((out (assoc-ref outputs "out"))
> +         (outbin (string-append out "/bin"))
> +         (outlib (string-append out "/lib/dub/" (basename out))))

In other places these variables are just called “bin” and “lib”.

> +    (mkdir-p outbin)
> +    ;; TODO remove "-test-application"
> +    (copy-recursively "bin" outbin)
> +    (mkdir-p outlib)
> +    (delete-file-recursively "vendor") ; contains timestamps
> +    (copy-recursively "." (string-append outlib))

(string-append outlib) -> outlib.

> +(define* (dub-build #:key inputs (phases %standard-phases)
> +                      #:allow-other-keys #:rest args)
> +  "Build the given DUB package, applying all of PHASES in order."
> +  (apply gnu:gnu-build #:inputs inputs #:phases phases args))

You can remove this one since it’s unused.

Thank you!

Ludo’.