Re: [PATCH] gnu: Add sassc.

[Mark H Weaver]

David Thompson <address@hidden> writes:

> From 4a6d1a798a0625d8bc104b86db56f5d2594a5d80 Mon Sep 17 00:00:00 2001
> From: David Thompson <address@hidden>
> Date: Wed, 19 Aug 2015 21:54:57 -0400
> Subject: [PATCH] gnu: Add sassc.
>
> * gnu/packages/web.scm (sassc): New variable.
> ---
>  gnu/packages/web.scm | 53 
> ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 53 insertions(+)
>
> diff --git a/gnu/packages/web.scm b/gnu/packages/web.scm
> index 48bfbc7..ce4cfeb 100644
> --- a/gnu/packages/web.scm
> +++ b/gnu/packages/web.scm
> @@ -642,6 +642,59 @@ minimum to provide high performance operation.")
>      ;; bundled CuTest framework uses a different non-copyleft license.
>      (license (list l:asl2.0 (l:non-copyleft 
> "file://test/CuTest-README.txt")))))
>  
> +(define-public sassc
> +  ;; libsass must be statically linked and it isn't included in source the
> +  ;; sassc release tarballs, hence this odd package recipe.
> +  (let* ((version "3.2.5")
> +         (libsass
> +          (origin
> +            (method url-fetch)
> +            (uri (string-append
> +                  "https://github.com/sass/libsass/archive/";
> +                  version ".tar.gz"))
> +            (file-name (string-append "libsass-" version ".tar.gz"))
> +            (sha256
> +             (base32
> +              "1x25k6p1s1yzsdpzb7bzh8japilmi1mk3z96q66pycbinj9z9is4")))))
> +    (package
> +      (name "sassc")
> +      (version version)
> +      (source (origin
> +                (method url-fetch)
> +                (uri (string-append "https://github.com/sass/sassc/archive/";
> +                                    version ".tar.gz"))

Please add a 'file-name' field here.

> +                (sha256
> +                 (base32
> +                  "1xf3w75w840rj0nx375rxi7mcv1ngqqq8p3zrzjlyx8jfpnldmv5"))))
> +      (build-system gnu-build-system)
> +      (arguments
> +       `(#:make-flags '("CC=gcc")
> +         #:test-target "test"
> +         #:phases
> +         (modify-phases %standard-phases
> +           (delete 'configure)
> +           (add-before 'build 'set-libsass-path
> +             (lambda* (#:key inputs #:allow-other-keys)
> +               (system* "tar" "xvf" (assoc-ref inputs "libsass"))

I think this phase should be renamed, since it is doing much more than
just setting the path.  How about 'unpack-libsass-and-set-path'?

The return value of 'system*' should be checked.

Also, I think this pass should go after 'unpack' instead of before
'build', so that the other passes between 'unpack' and 'build' can do
their jobs.  It's possible that it doesn't make a difference in this
case, but that might change in a future version of libsass, and anyway
since copy+paste from existing packages is a common method, we should
encourage best practices.

> +               (setenv "SASS_LIBSASS_PATH"
> +                       (string-append (getcwd) "/libsass-" ,version))
> +
> +               #t))

I'd probably remove that blank line here.

> +           (replace 'install ; no install target
> +             (lambda* (#:key outputs #:allow-other-keys)
> +               (let ((bin (string-append (assoc-ref outputs "out") "/bin")))
> +                 (mkdir-p bin)
> +                 (copy-file "bin/sassc"
> +                            (string-append bin "/sassc"))
> +                 #t))))))
> +      (inputs
> +       `(("libsass" ,libsass)))
> +      (synopsis "CSS pre-processor")
> +      (description "SassC is a compiler written in C for the CSS 
> pre-processor
> +language known as SASS.")
> +      (home-page "http://sass-lang.com/libsass";)
> +      (license l:expat))))

Otherwise it looks good to me.  Thanks!

     Mark