Sriram Natarajan wrote: > > > Pradhap Devarajan wrote: >> Quick run through. Remove executable permissions from "install-sfw" >> and "man-fix" script. Otherwise looks ok to me. > Just curious - How does removing the 'executing' permission is > supposed to make this a better integration or reduce the maintenance > overhead . Some time, I am quite puzzled with some of the review > comments that I see in this email thread making me wonder do I ever > want to integrate in SFW. As a general comment, there have been some reviewers here in the last couple of years who have set the bar really high for code reviews. The result is that the standard of initial code review requests is really high now, the key points for submitters are well established and can be gleaned from looking through the alias or at other submissions in the workspace. Code reviews are generally easier to read, there is little need to nit pick about copyright headers in source files and other formatting issues. Ultimately it leaves code reviewers free to review the code and not worry about nits or worry about missing issues obfuscated by poorly presented code. Maybe that's something you only get a feel for if you do a lot of code reviews.
As for the execute permissions, webrev picked that up and flagged it as well, it's highlighted in orange. Amanda > > - Sriram >> >> Amanda Waite wrote: >>> Looks ok to me. >>> >>> Amanda >>> >>> Jan Forch wrote: >>>> Hi Amanda, >>>> thanks for prompt answer. See the comments in lines below. Please >>>> let us know asap whether you are ok with this. >>>> Thanks a lot, >>>> Jan >>>> >>>> webrev: http://cr.opensolaris.org/~jf222792/sfwnv_w/ >>>> >>>> On 08/12/09 18:14, Amanda Waite wrote: >>>>> You overwrote the sfwnv_p version of the webrev so I can't see >>>>> what's changed since the last time I reviewed it. >>>>> >>>>> 1. usr/src/pkgdefs/SUNWfreeipmir/Makefile >>>>> >>>>> You have 'depend' listed under DATAFILES, this will pull in the >>>>> default depend file at build time and probably overwrite yours. >>>> FIXED >>>>> >>>>> 2. usr/src/pkgdefs/SUNWfreeipmir/depend >>>>> >>>>> Out of curiousity, why does the Root package need to depend on >>>>> SUNWesu? If it's an absolute requirement for freeipmi then >>>>> shouldn't it be a dependency for the Usr package? >>>> > make check_deps >>>> >>>> ==== Checking package dependencies ==== >>>> >>>> ... >>>> >>>> **** /lib/svc/method/svc-bmc-watchdog (SUNWfreeipmir) requires >>>> /usr/bin/nawk which is found in SUNWesu, but that is not >>>> listed as a >>>> package dependency. >>>> *** /lib/svc/method/svc-ipmidetectd (SUNWfreeipmir) requires >>>> /usr/bin/nawk >>>> which is found in SUNWesu, but that is not listed as a package >>>> dependency.* >>>> >>>> ... >>>> >>>> WE WERE GETTING NAMED DEPENDENCY ERROR MSG FOR ROOT PKG BUT NOT FOR >>>> USER PKG ONCE WE ADDED DEPENDENCY IN DEPEND FILE ERROR WERE GONE. >>>> THE REASON IS >>>> */lib/svc/method/svc-bmc-watchdog and >>>> **/lib/svc/method/svc-ipmidetectd are using **/usr/bin/nawk >>>> presented in **SUNWesu pkg* >>>>> >>>>> 3. usr/src/cmd/freeipmi/Makefile.sfw >>>>> >>>>> you've removed the bmi_intf.h from the workspace but haven't >>>>> removed ${SRC}/cmd/freeipmi from the include path in $(CPPFLAGS). >>>> REMOVED >>>>> >>>>> 4. usr/src/cmd/freeipmi/sunman-stability >>>>> >>>>> bmc-config --checkout may not work properly because of Sun's BMC >>>>> driver issue. >>>>> >>>>> bmc-device --get-lan-statistics may not work properly because of >>>>> Sun's BMC issue. >>>>> >>>>> You need to add these to the "known issues" list, not just append >>>>> them to the bottom of the file, patch the man page files. Also the >>>>> wording "Sun's BMC driver issue" I don't think is going to fly. >>>>> Try "issues with some versions of the BMC driver" or something >>>>> like that. >>>> FIXED >>>>> >>>>> 5. Where's the Source tarball? It seems to have gone missing >>>> IT WAS JUST MISSING FROM WEBREV. FIXED. >>>>> >>>>> Amanda >>>>> >>>>> >>>>> >>>>> >>>>> Jan Forch wrote: >>>>>> Hi folks, >>>>>> please could someone do webrev for us. It is really urgent. We >>>>>> would like to be integrated into Fridays build. Thanks, >>>>>> Jan >>>>>> >>>>>> Webrev: http://cr.opensolaris.org/~jf222792/sfwnv_w/ >>>>> >>>> >>> >>> _______________________________________________ >>> 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
