Amanda, Looks okay to me
Paul Amanda Waite wrote: > Hi Paul, > > Mike Sullivan advised me that the extra protofix lines may have been > added by them and are required for incrementals. He was right so I've > put them back. While I was at it I found that the patches were being > applied twice during incrementals so I manipulated the make targets a > little. > > http://cr.opensolaris.org/~tekgrrl/CR-6871383-01/ > > Has the changes, the only chnages are in $SRC/cmd/ruby18/Makefile.sfw > > Could you possibly look over them for me? > > In the meantime can someone else review the webrev please? > > Also, how do you change permissions on a file under SCCS control? Last > time, I had to remove the file from SCCS and recreate it. > > Thanks > > Amanda > > > Amanda Waite wrote: >> 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 >> >> _______________________________________________ >> sfwnv-discuss mailing list >> sfwnv-discuss at opensolaris.org >> http://mail.opensolaris.org/mailman/listinfo/sfwnv-discuss > > _______________________________________________ > sfwnv-discuss mailing list > sfwnv-discuss at opensolaris.org > http://mail.opensolaris.org/mailman/listinfo/sfwnv-discuss > -- Paul Cunningham Software Engineer Tel: 01462 685974
