Hi Prashant, thank you for your review. My comments are inline.
With best regards, Evgeny Prashant Srinivasan wrote: > Evgeny, > > Comments, some code review, some architecture related, and some > unsolicited advice :-) > > -> ant is already in SFWNV - it may be good to check if you can just > leverage that. I see that sfw has 1.6.5 (and you're planning on > 1.7.1). It'll be nice to investigate if we can avoid having multiple > copies of ant on the file system. ?vgeny> Build of JRuby requires ant1.7.x (ant1.6.5 has no some tasks) > -> The Netbeans folk had an ARC case for Netbeans JRuby support a few > months ago. Are you in synch with them? If not, then we're probably > going to have 2 versions of JRuby in SFWNV. ?vgeny> Yes. Porting of JRuby into OpenSolaris was initiated by NetBeans management. > -> /usr file system is not a good place for the gem_home since > administrators in sparse root zones will not be able to use it. We > chose /var in the Ruby (MRI) ARC case. > > -> I see that you're bundling Rubygems and the rake gem. We ran into > issues with this - the main issue being the dual ownership of gems. > ie., Solaris packages and the gem system own the same bits, and so at > any given time you're never sure what exactly your package contents > are. Look through the webstack-discuss archives for details (we really > beat this topic to death, and then some more on that mailing list). > Amanda Waite is looking into an 'enlightened' hierarchical way of doing > this now, which makes some assumptions, but could be workable. It'll be > good to get in touch with her. > > -> It's probably not a good idea to bundle Rubygems and JRuby in the > same package, since, sometimes user level programs depend on a specific > Rubygems version(like the current version of Rails), and if you don't > have a versioned Rubygems package, then your users will have no way to > pick an updated version of Rubygems that you may put up on > pkg.opensolaris.org. > > An alternate solution may be to have some sort of composite versioning > for your package that takes into account the rev number of both Rubygems > and JRuby. ?vgeny> JRuby has it a little bit different. At the moment JRuby can only see the gems installed with the gem command shipped with JRuby. The gems installed this way will be stored under $JRUBY_HOME/lib/ruby. Also it's bundled on http://jruby.org/ and I see no way how to separate it into different packages. Anyway studing webstack-discuss... > -> The sunman-stability file usually says that the sources for XYZ are > available at http://opensolaris.org. I believe that that is the case > since we may, at times, deliver code that has third party patches or > just code that has not made it upstream(and hence deliver a different > code base with sfwnv than what's in the community). So if that's the > case with you, this line may be worth changing(Mike Sullivan may be able > to comment on this more.) ?vgeny> OK. I've changed sunman-stability file. http://cr.opensolaris.org/~jinb/jruby5 > Hope that helps > -ps > > > > > On 09/ 9/08 06:58 AM, Evgeny Bessonov wrote: > >>Paul, thanks a lot for your review. >> >>Since there is a requirement of at least two code reviewers >>do somebody else has a few minutes for code review of SUNWjruby? >> >>http://cr.opensolaris.org/~jinb/jruby3/ >> >>Thanks a lot. >>With best regards. >>Evgeny >> >> >>Paul Cunningham wrote: >> >> >> >>>Evgeny, >>> >>>Evgeny Bessonov wrote: >>> >>> >>> >>>>thank you very much for explanations. >>>> >>>>I removed rm/mkdir, edited/moved back DESC >>>>and set 444 permissions for files. >>>> >>>>Updated version of JRuby webrev available here: >>>>http://cr.opensolaris.org/~jinb/jruby3/ >>>> >>>> >>>This looks okay to me now >>> >>>Paul >>> >>> >> >>_______________________________________________ >>sfwnv-discuss mailing list >>sfwnv-discuss at opensolaris.org >>http://mail.opensolaris.org/mailman/listinfo/sfwnv-discuss >> >> > > > -- > Prashant Srinivasan > F/OSS Enthusiast > Sun Microsystems, Inc. > http://blogs.sun.com/prashant > GnuPG key: http://pgp.mit.edu:11371/pks/lookup?op=get&search=0x82FBDE5A >
