Hello, On 11/05/11 21:24, Antonio Terceiro wrote: >> OK. I've reverted the commits I had pushed to master, and stored them >> on the setup-rb branch. I'll merge any change necessary from your commits. > > Ok, I've pushed my master branch there. It would be nice if you could > provide a clean set of changes on top of that, with: > > * one commit with the refactoring of the destdir calculations (that's > a nice catch, thanks!) > * one commit that adds the new class, the corresponding change in > the dh_ruby script for intantiating the correct class. ... I've split the later one, is it a huge deal ? I like small-but-consistent commits (all intermediate commits run fine). > About the code: > > Please add your new class to a new file with a proper name and add the > proper require in dh_ruby. Done. > It would be nice to have automated tests for this new class. Hmmmm... A bit later ? > The spawn method you wrote already exists: run(cmd). You can use it > instead. Thanks. > Instead of adding a new debhelper buildsystem, would't it be better to > centralize the logic of calling dh_ruby in one place? I'm not sure about > this, because that code seems so simple already. :-/ I prefer a separate buildsystem since I don't want the setuprb one to trigger automatically (that would wreck havoc on the buildds, I believe), and I don't see how this could be done simply inside the dh_ruby thing. Could this be a variable from within debian/rules ? Is there an API for reading those ? If not, that means adding quite a bit of code to dh_ruby to parse debian/rules, I'm not sure we want that. For now, the attached patches keep with my first solution of a separate build system. > Your internal_install method doesn't handle running the tests. It would > be nice to extract the logic of running tests into a method of its on, > so that the install method would look like the following sequence: > > * install files and build extensions > * run tests > * update shebangs > * check for require rubygems > > then you would only need to override the install files and build > extensions part. Actually, check the updated master branch, it's easier > to show than than to describe. :) Yes, that's perfect ! This way, the logic is clear and overriding is easy. > http://git.debian.org/?p=pkg-ruby-extras/gem2deb.git;a=commitdiff;h=fb547d84b5545ac5dee8de9023ccae51d5cf61da > > So you should be good by overriding install_files_and_build_extensions > on your new class. I've done that, and did as well for tests so far, but just stating it is currently not supported. I need to dig a little more into this. > after (or instead of) `setup.rb clean`, shouldn't you also do `setup.rb distclean`? > (I think ruby-pkg-tools does something like that) I've changed to distclean. I've attached an archive of git format-patch, to avoid blundering too much with the master repository. Cheers, Vincent -- Vincent Fourmond, Debian Developer http://vince-debian.blogspot.com/ This thing was [...] incessantly making that flat honking noise of the sort duck hunters make just before they are shot by other duck hunters. -- Terry Pratchet, Unseen Academicals Vincent, listening to Mirrorage (Glasser)
Attachment:
gem2deb-setuprb-patches.tar.gz
Description: application/gzip