From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ben Woodcroft Subject: Re: [PATCH] Gemspecs / Add ruby-ruby-engine. Date: Tue, 5 Jan 2016 22:09:02 +1000 Message-ID: <568BB25E.3040805@uq.edu.au> References: <5683842C.3090706@uq.edu.au> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: quoted-printable Return-path: Received: from eggs.gnu.org ([2001:4830:134:3::10]:50506) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aGQQQ-00014B-3e for guix-devel@gnu.org; Tue, 05 Jan 2016 07:09:19 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aGQQM-0008Re-RY for guix-devel@gnu.org; Tue, 05 Jan 2016 07:09:18 -0500 Received: from mailhub1.soe.uq.edu.au ([130.102.132.208]:34951 helo=newmailhub.uq.edu.au) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aGQQM-0008RH-8q for guix-devel@gnu.org; Tue, 05 Jan 2016 07:09:14 -0500 In-Reply-To: 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: Ricardo Wurmus Cc: "guix-devel@gnu.org" On 05/01/16 21:36, Ricardo Wurmus wrote: > Ben Woodcroft writes: [..] >> While I managed to install 1.0.1, I wasn't sure how best to remove the >> bundled 1.0.0 .gem file. The issue is that when the source is a .gem >> file (ie most of the time), the gemspec is taken from the downloaded >> .gem file directly, and in the same phase the .gem file is built. So a= s >> a packager there is no way to make changes to the gemspec without >> replacing the entire build phase. There's a number of rubygems that ar= e >> contaminated with junk so it would be great for there to be a simple w= ay >> to modify the gemspec before "gem build" is run. > The =E2=80=9Cbuild=E2=80=9D phase in the =E2=80=9Cruby-build-system=E2=80= =9D is responsible for > rebuilding the gem from source. The =E2=80=9Cunpack=E2=80=9D phase unp= acks the gem > archive. This should allow you to modify the gemspec in a phase > injected between =E2=80=9Cunpack=E2=80=9D and =E2=80=9Cbuild=E2=80=9D, = no? That's not what I'm getting from reading of the code, no. The build=20 phase of the ruby build system unpacks the gemspec from the source .gem=20 file and then immediately uses it to build the gem. So if a file is=20 deleted in a snippet or otherwise then the "gem build" command fails=20 because it cannot find the deleted file - the gemspec contains a list of=20 files to include in the gem. Does that make sense? My suggestion is to=20 add a gemspec phase before build so that packagers are given the=20 opportunity to modify the gemspec without having to rewrite the entire=20 build phase. >> Would someone with more experience like to suggest a way of doing this= ? >> A new "gemspec" phase before "build" that takes the gemspec out of the >> .gem so the packager can manipulate it perhaps? >> >> It would also be good to check that there is only one .gem file. > And do what when this check fails? If included gems were removed in a > snippet they would never be seen at a later point, so I think the right > way to do this is support snippets. Does this make sense? It is fine to remove the offending files but the gemspec must be=20 modified accordingly otherwise "gem build" fails. > > Now on to the patch: > >> + >> +(define-public ruby-ruby-engine >> + (package >> + (name "ruby-ruby-engine") >> + (version "1.0.1") >> + (source >> + (origin >> + (method url-fetch) >> + (uri (rubygems-uri "ruby_engine" version)) >> + (sha256 >> + (base32 >> + "1d0sd4q50zkcqhr395wj1wpn2ql52r0fpwhzjfvi1bljml7k546v")))) >> + (build-system ruby-build-system) >> + (arguments >> + `(#:phases >> + (modify-phases %standard-phases >> + (add-before 'check 'clean-up > Is it possible to move this after =E2=80=9Cunpack=E2=80=9D instead? It= =E2=80=99s just a > side-effect of the =E2=80=9Ccheck=E2=80=9D phase that the gem is instal= led, so I think > it=E2=80=99s best to move this phase right after =E2=80=9Cunpack=E2=80=9D= (because we don=E2=80=99t need > any of this stuff for any of the phases until =E2=80=9Ccheck=E2=80=9D). > > Maybe you can also add a FIXME comment (as in =E2=80=9Cruby-pygmentize=E2= =80=9D) stating > that this really should be done in a snippet. Unfortunately we cannot move it since the build phase will then fail for=20 the above reason. > >> + (lambda _ >> + (delete-file "Gemfile.lock") >> + (substitute* "ruby_engine.gemspec" >> + ;; Remove unnecessary imports that would entail furthe= r >> + ;; dependencies. >> + ((".*> + ((".*> + ;; Remove extraneous .gem file >> + (("\\\"pkg/ruby_engine-1.0.0.gem\\\",") "")) >> + (substitute* "Rakefile" >> + (("require 'rubygems/tasks'") "") >> + (("Gem::Tasks.new") "")) >> + ;; Remove extraneous .gem file that otherwise gets insta= lled. >> + (delete-file "pkg/ruby_engine-1.0.0.gem") >> + #t))))) >> + (native-inputs >> + `(("bundler" ,bundler) >> + ("ruby-rspec" ,ruby-rspec-2))) >> + (synopsis "Simplifies checking for Ruby implementation") >> + (description >> + "@code{ruby_engine} provides an RubyEngine class that can be use= d to check >> +which implementation of Ruby is in use. It can provide the interpret= er name and >> +provides query methods such as @{RubyEngine.mri?}.") > =E2=80=9Cruby_engine=E2=80=9D is a name, so I would not use @code here.= How about this: > > The ruby_engine package provides a @code{RubyEngine} class that can = be > used to check which implementation of Ruby is in use. ... ok > >> + (home-page >> + "https://github.com/janlelis/ruby_engine") > Please keep this on one line. > Otherwise it=E2=80=99s fine. Thank you! No problem. I'm happy to send a follow up patch if you like, but would=20 prefer to resolve the larger problem first. Thanks for the review. ben