Paul Cunningham wrote: > Amanda, > > This mainly looks good to me, but below are a few observations (feel > free to ignore) ... > > > Amanda Waite wrote: >> >> Can I get a code review for an update to Ruby 1.8.7. This updates >> Ruby 1.8.7 to patch level 174 and updates RubyGems to v1.3.5. >> >> The webrev is here: http://cr.opensolaris.org/~tekgrrl/CR-6871383 >> >> The patch file ${SRC}/cmd/ruby18/patches/ruby_nogdbm.patch had to be >> removed and re-added so as to reset its permissions. > > 1. usr/src/cmd/ruby18/Makefile.sfw > Why is the 'protofix' done under the 'all: rule rather than > under the 'install:' rule? > > Why are ruby and rubygem builds done under the 'install-*' rules > rather than under the 'all:' or another rule(s)? (ie. separating > them from the 'install')
Much of it is legacy, I didn't want to change what already worked. There were two places where protofix was being run, one of them before anything was even built. I basically just removed that and cleaned up the configure options. It works good so I probably won't change it unless you have strenuous objections. > > Line 95, maybe the 'env ' should be 'env - ' In this case it's predictable which environment variables will be used by the script. With configure scripts and Makefiles it's not so straight forward and it's best to clean out the environment before calling them, that's not the case when calling the install-sfw script. I can change it if you want me to. > > > 2. usr/src/pkgdefs/SUNWruby18u/prototype_com > Should you have the write-permission bit set on the files in here? These are all under /usr/ruby/1.8, I think that we've covered all the files in the standard /usr paths. These are the permissions that Ruby causes to be set during the build and IMO it would be a mistake to change them, it would also be a lot of work and potentially error-prone. This works today and it worked before so I've no plans to change them. Thanks for the review Amanda > > > Paul
