From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ricardo Wurmus Subject: Re: [PATCH] gnu: Add ruby-nokogiri Date: Mon, 13 Jul 2015 15:24:20 +0200 Message-ID: <877fq4i41n.fsf@elephly.net> References: <20150713130956.GC28969@thebird.nl> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Return-path: Received: from eggs.gnu.org ([2001:4830:134:3::10]:53435) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZEdie-0004oO-7M for guix-devel@gnu.org; Mon, 13 Jul 2015 09:24:29 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZEdia-0001TF-3b for guix-devel@gnu.org; Mon, 13 Jul 2015 09:24:28 -0400 Received: from sender163-mail.zoho.com ([74.201.84.163]:25885) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZEdiZ-0001T8-Qs for guix-devel@gnu.org; Mon, 13 Jul 2015 09:24:24 -0400 In-reply-to: <20150713130956.GC28969@thebird.nl> List-Id: "Development of GNU Guix and the GNU System distribution." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: guix-devel-bounces+gcggd-guix-devel=m.gmane.org@gnu.org Sender: guix-devel-bounces+gcggd-guix-devel=m.gmane.org@gnu.org To: Pjotr Prins Cc: "guix-devel@gnu.org" Hi Pjotr, following are a couple of comments about the patch. (I did not try it myself.) > From e2fd935a659cacde5cb74bef19406f056a262f79 Mon Sep 17 00:00:00 2001 > From: pjotrp > Date: Mon, 13 Jul 2015 14:56:40 +0200 > Subject: [PATCH] gnu: Add ruby-nokogiri > * gnu/packages/ruby.scm (gnu-nokogiri): New variable The name of the variable is ‘ruby-nokogiri’, not ‘gnu-nokogiri’. There also should be a period at the end. > +(define-public ruby-nokogiri > + (package > + (name "ruby-nokogiri") > + (version "1.6.6.2") > + (source (origin > + (method url-fetch) > + (uri (string-append > + "https://github.com/sparklemotion/nokogiri/archive/v" > + version ".tar.gz")) > + (file-name (string-append name "-" version ".tar.gz")) > + (patches (map search-patch > + (list "ruby-nokogiri-Rakefile.patch"))) ‘(patches ...)’ should be aligned with the previous line. ‘(list’ should be aligned with ‘search-patch’. Since you only have one patch, though, I think using ‘(map search-patch’ is odd. > + (arguments > + '( > + #:tests? #f ;; test fails because nokogiri can only test with a built extension (now part of install phase) ‘'(’ should not be on a line of its own. The margin comment should only have one semicolon and the text should be broken up to avoid long lines. > + #:gem-flags (list "--use-system-libraries" (string-append "--with-xml2-include=" (assoc-ref %build-inputs "libxml2") "/include/libxml2" )) This is a very long line. It can be broken up easily. > + #:phases (alist-replace > + 'build > + (lambda _ > + ;; calling rake gem 2x begets a gem > + (system* "rake" "gem") > + (zero? (system* "rake" "gem"))) > + %standard-phases))) The alignment here is off. Also consider using the ‘modify-phases’ syntax instead. Why exactly is “rake gem” called twice? What is different the second time around? > + (native-inputs > + `(("ruby-hoe" ,ruby-hoe) > + ("ruby-rake-compiler", ruby-rake-compiler))) > + (inputs > + `(("zlib" ,zlib) > + ("libxml2" ,libxml2) > + ("libxslt" ,libxslt))) > + (synopsis "Nokogiri (鋸) is an HTML, XML, SAX, and Reader parser") > + (description "Nokogiri parses and searches XML/HTML very quickly, and also has correctly implemented CSS3 selector support as well as XPath 1.0 support.") Please break apart the long description into multiple lines. In Emacs you can do this easily with ‘fill-paragraph’ (bound to ‘M-q’ by default). > + (home-page "http://www.nokogiri.org/") > + (license license:x11))) > + ~~ Ricardo