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 > > Paul Cunningham wrote: >> >> Evgeny Bessonov wrote: >> >>> >>> >> 3. usr/src/cmd/jruby/install-jruby >>> >> You don't need the 'mkdir -p ..' stuff as the dir >>> >> is already in Targetdirs. >>> > Doesn't look as though you changed this >>> >>> But prev. line(36) contains rm ?rf the-same-dir, so it should be >>> recreated after possible removal (mkdir ?p on next line 37). >> >> >> I meant you should remove the 'rm -rf ...' as well. >> The build process ensures that all the directories listed in >> 'Targetdirs' are created at the start of the build - so if your dir is >> in there you shouldn't need the 'rm' + 'mkdir' lines. >> >>> >>> >> 8. usr/src/pkgdefs/SUNWjruby/pkginfo.tmpl >>> >> Some people put the pkg version at the end of the >>> >> DESC= line (feel free to ignore this) >>> > I think you probably misunderstood what I meant; >>> > I don't mean 'move the DESC= line to the end of the file', >>> > what I meant was 'add something like >>> > the following to that line' ... >>> > DESC="Java implementation of the Ruby programming language (1.1.3)" >>> > and it's probably better to move the line back to >>> > where it was (rather than it the end of the file). >>> >>> Yes. Sure. It's my fault. I'll add version and move DESC back. >>> >>> >> 9. usr/src/pkgdefs/SUNWjruby/prototype_com >>> >> Do all these files really need the write permissions >>> >> bit set? >>> > What was your conclusion on this? >>> >>> As I see in packages tree there are a lot of examples of 755 for >>> folders/ runable stuff and 644 permissions set for the rest. >> >> >> Yes I know, but you don't need the write permissions on the files so >> why have them (unless you really need them) >> >> Paul >> >>> >>> Paul Cunningham wrote: >>> >>>> Evgeny, >>>> >>>> I took another quick look .... >>>> >>>> >> 3. usr/src/cmd/jruby/install-jruby >>>> >> You don't need the 'mkdir -p ..' stuff as the dir >>>> >> is already in Targetdirs. >>>> Doesn't look as though you changed this >>>> >>>> >> 8. usr/src/pkgdefs/SUNWjruby/pkginfo.tmpl >>>> >> Some people put the pkg version at the end of the >>>> >> DESC= line (feel free to ignore this) >>>> I think you probably misunderstood what I meant; I don't mean 'move >>>> the DESC= line to the end of the file', what I meant was 'add >>>> something like the following to that line' ... >>>> DESC="Java implementation of the Ruby programming language (1.1.3)" >>>> and it's probably better to move the line back to where it was >>>> (rather than it the end of the file). >>>> >>>> >> 9. usr/src/pkgdefs/SUNWjruby/prototype_com >>>> >> Do all these files really need the write permissions >>>> >> bit set? >>>> What was your conclusion on this? >>>> >>>> >>>> The rest looks okay to me >>>> >>>> Paul >>>> >>>> >>>> Evgeny Bessonov wrote: >>>> >>>>> Hi Paul, >>>>> >>>>> thank you for your review! >>>>> >>>>> Code was fixed according to your/ Roland's comments >>>>> with exception of p3.3 (cp vs _install). >>>>> I prefer to stay with cp because a lot of >>>>> stuff (11668 files/ 1858 folders) should be copied. >>>>> I've updated webrev page: >>>>> >>>>> http://cr.opensolaris.org/~jinb/jruby2 >>>>> >>>>> Is it better now? >>>>> >>>>> Thanks a lot. >>>>> With best regards >>>>> Evgeny >>>>> >>>>> >>>>> Paul Cunningham wrote: >>>>> >>>>>> Evgeny, >>>>>> >>>>>> See below for my comments on this ... >>>>>> >>>>>> Paul >>>>>> >>>>>> Evgeny Bessonov wrote: >>>>>> >>>>>>> >>>>>>> I'm working on poring JRuby (Java implementation >>>>>>> of the Ruby language) 1.1.3 into OpenSolaris. >>>>>>> And here is webrev for jruby/SUNWjruby: >>>>>>> http://cr.opensolaris.org/~jinb/jruby1/ >>>>>>> >>>>>>> We have not enough time till svn_99/100 gate closing >>>>>>> so could you please review/ approve/ provide your feedback/notes >>>>>>> ASAP? >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> === Start of Comments ==== >>>>>> >>>>>> 1. usr/src/cmd/jruby/METADATA >>>>>> Add line to say where the source code came from 'URL:...' >>>>>> >>>>>> 2. usr/src/cmd/jruby/Makefile.sfw >>>>>> You also might want to use 'env - ' rather than 'env ' >>>>>> >>>>>> 3. usr/src/cmd/jruby/install-jruby >>>>>> You might want to >>>>>> look at 'Roland Mainz' comments on other people's >>>>>> install-* scripts, re the shell used and . command, >>>>>> and apply those comments - look back through >>>>>> sfwnv-discuss for them. >>>>>> >>>>>> You don't need the 'mkdir -p ..' stuff as the dir >>>>>> is already in Targetdirs. >>>>>> >>>>>> Do the files copied into proto with 'cp' need their >>>>>> permissions set? Why not do it with '_install N ...'. >>>>>> >>>>>> 4. usr/src/cmd/jruby/test >>>>>> Where is this used ? >>>>>> >>>>>> 5. usr/src/pkgdefs/SUNWjruby/Makefile >>>>>> Copyright year is wrong? >>>>>> >>>>>> You could remove the null DATAFILES= line >>>>>> >>>>>> 6. usr/src/pkgdefs/SUNWjruby/depend >>>>>> Copyright year is wrong? >>>>>> >>>>>> Move copyright lines to after 'CDDL HEADER END' >>>>>> header. >>>>>> >>>>>> 7. usr/src/pkgdefs/SUNWjruby/pkginfo >>>>>> You don't need this checked-in as it is auto >>>>>> generated from SUNWjruby/pkginfo.tmpl >>>>>> >>>>>> 8. usr/src/pkgdefs/SUNWjruby/pkginfo.tmpl >>>>>> Some people put the pkg version at the end of the >>>>>> DESC= line (feel free to ignore this) >>>>>> >>>>>> 9. usr/src/pkgdefs/SUNWjruby/prototype_com >>>>>> Do all these files really need the write permissions >>>>>> bit set? >>>>>> >>>>>> Format and year of the copyright lines are wrong >>>>>> >>>>>> 10. usr/src/pkgdefs/SUNWjruby/prototype_i386 >>>>>> & usr/src/pkgdefs/SUNWjruby/prototype_sparc >>>>>> Copyright year is wrong? >>>>>> >>>>>> === End of Comments ====== >>>>>> >>>> >> -- ---------------------------------------------------------------------- Paul Cunningham Software Engineer Tadpole Computer Products
