Re: [PATCH] Gemspecs / Add ruby-ruby-engine.

[Ben Woodcroft]

On 05/01/16 21:36, Ricardo Wurmus wrote:
> Ben Woodcroft <address@hidden> 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 as
> > 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 are
> > contaminated with junk so it would be great for there to be a simple way
> > to modify the gemspec before "gem build" is run.
> The “build” phase in the “ruby-build-system” is responsible for
> rebuilding the gem from source.  The “unpack” phase unpacks the gem
> archive.  This should allow you to modify the gemspec in a phase
> injected between “unpack” and “build”, no?
That's not what I'm getting from reading of the code, no. The build 
phase of the ruby build system unpacks the gemspec from the source .gem 
file and then immediately uses it to build the gem. So if a file is 
deleted in a snippet or otherwise then the "gem build" command fails 
because it cannot find the deleted file - the gemspec contains a list of 
files to include in the gem. Does that make sense? My suggestion is to 
add a gemspec phase before build so that packagers are given the 
opportunity to modify the gemspec without having to rewrite the entire 
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 
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 “unpack” instead?  It’s just a
> side-effect of the “check” phase that the gem is installed, so I think
> it’s best to move this phase right after “unpack” (because we don’t need
> any of this stuff for any of the phases until “check”).
>
> Maybe you can also add a FIXME comment (as in “ruby-pygmentize”) stating
> that this really should be done in a snippet.
Unfortunately we cannot move it since the build phase will then fail for 
the above reason.
> > +           (lambda _
> > +             (delete-file "Gemfile.lock")
> > +             (substitute* "ruby_engine.gemspec"
> > +               ;; Remove unnecessary imports that would entail further
> > +               ;; dependencies.
> > +               ((".*<rdoc.*") "")
> > +               ((".*<rubygems-tasks.*") "")
> > +               ;; 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 installed.
> > +             (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 used to check
> > +which implementation of Ruby is in use.  It can provide the interpreter name 
> > and
> > +provides query methods such as @{RubyEngine.mri?}.")
> “ruby_engine” 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’s fine.  Thank you!
No problem. I'm happy to send a follow up patch if you like, but would 
prefer to resolve the larger problem first.

Thanks for the review.
ben