I've not heard anything back, so do we consider the code reviewed. Bruce
On Sep 2, 2008, at 4:16 PM, Bruce Rothermal wrote: > > If I remove the cd .. the target execution of install-swig does not > work. I does not copy all the doc files to the proto area. > > gmake[1]: Leaving directory `/net/dv1/vol/wsvol/ws1/brucer/sfw/ws1/ > usr/src/cmd/swig/swig-1.3.35' > /usr/bin/sh ./install-swig > # apply the file attributes from the packaging > for pkg in SUNWswig ; do \ > /net/dv1/vol/wsvol/ws1/brucer/sfw/ws1/usr/src/tools/protofix -- > pkg $pkg --perm ; \ > done > > Bruce > > Norm Jacobs wrote: >> usr/src/cmd/subversion/Makefile.sfw >> line 179 -rm -rf $(SWIGVER) this line can go. I should have >> noticed it before. >> >> A couple more comments inline >> >> -Norm >> >> Bruce Rothermal wrote: >> >>> Made the changes and posted them at http://cr.opensolaris.org/~bruce_r/swig/ >>> >>> Had a problem with this change at this point I'm already in $(VER) >>> and wish to cd to directory up out of the swig directory. >>> >>> * lines 55 and 56 should be (cd $(VER); $(SH) ./install-swig) >>> >> You shouldn't be in the $(VER) directory at this point, because the >> lines above it are executed in a subshell like "(cd $(VER); pwd) ; >> pwd" >> >>> I also have a question about CFLAGS="$(CFLAGS)". Since I'm using >>> gmake with the -e option don't all these options get passed anyway. >>> >> No, This line copies the value of the CFLAGS make variable into >> the calling environment so that gmake can be made aware of it. >> >>> Bruce >>> >>> Norm Jacobs wrote: >>> >>>> Sorry I didn't get this to you last night. I was sidetracked. >>>> At any rate, I took another pass through your webrev. It's >>>> looking better, but the following issues still exist. >>>> >>>> usr/src/Targetdirs >>>> usr/src/cmd/Makefile >>>> usr/src/cmd/subversion/Makefile.sfw >>>> usr/src/pkgdefs/Makefile >>>> >>>> All have mismerge errors in them that cause them to lose changes >>>> that aren't part of your code change. It looks like you updated >>>> your workspace and somehow while resolving the conflicts, you >>>> managed to mismerge these files, probably via filemerge. You >>>> should >>>> only see changes that you made for your project in the webrev. >>>> >>>> usr/src/cmd/swig/Makefile.sfw >>>> >>>> * line 33 should be CFLAGS += --norunpath >>>> * add CFLAGS="$(CFLAGS)" \ to the environment between lines 38 >>>> and 39. >>>> * add INSTALL="$(INSTALL_PROTO)" \ between lines 50 and 51. >>>> * lines 55 and 56 should be (cd $(VER); $(SH) ./install-swig). >>>> >>>> usr/src/pkgdefs/SUNWswig/prototype_com >>>> >>>> * Please don't install writable files in /usr (many of your >>>> files >>>> are 0644). If you fix the prototype_com file entries, proto- >>>> fix >>>> will make sure that the proto area is ok during the build. >>>> >>>> -Norm >>>> >>>> >>>> Bruce Rothermal wrote: >>>> >>>>> Made changes from review and some responses: >>>>> >>>>> http://cr.opensolaris.org/~bruce_r/swig/ >>>>> >>>>> usr/src/cmd/subversion/Makefile.sfw >>>>> >>>>> * remove line 35 >>>>> * remove a ".WAIT" from line 36 >>>>> * put lines 41, 60-61. 97-98, 134-135 back from the original >>>>> (this >>>>> appears to have been a mismerge and Mike will be unhappy if >>>>> you >>>>> undo his fix :-( ) >>>>> * remove the SWIG related contents from lines 171-185, 189 >>>>> *All Done.* >>>>> >>>>> usr/src/cmd/subversion/swig* >>>>> >>>>> * remove (use "wx rm ..." to do this) >>>>> *Done >>>>> >>>>> *usr/src/cmd/swig/Makefile.sfw >>>>> >>>>> * line 33, why are you overriding "--with-swiglibdir" ? Is the >>>>> default value somehow broken? >>>>> DESTDIR=$(ROOT) fixes >>>>> * line 38, make sure that you are linking against the proto >>>>> area, >>>>> make sure you add --norunpath and check if you need the >>>>> standard >>>>> C++ libraries. >>>>> Add to CFLAGS >>>>> >>>>> * lines 38-41, add CFLAGS="$(CFLAGS)" to the environment. >>>>> passed via -e in call to gmake >>>>> >>>>> * line 45, remove >>>>> done >>>>> >>>>> * lines 48-52, remove >>>>> done >>>>> >>>>> * lines 56-57, add "INSTALL=$(INSTALL_PROTO)" and MANSCRIPT to >>>>> your >>>>> environment >>>>> MANSCRIPT done >>>>> Adding INSTALL=$(INSTALL_PROTO) causes configure to error and >>>>> exit. >>>>> >>>>> * lines 59-60, remove and use proto-fix to get the perms right >>>>> on >>>>> the proto area (see a2ps, hpijs, cups, or other component >>>>> for an >>>>> example) >>>>> Added the proto-fix >>>>> >>>>> usr/src/cmd/swig/install-swig >>>>> >>>>> * incorporate the swig.1 manpage install into Makefile.sfw (and >>>>> anything else that "cd $VER; make install" didn't cover) and >>>>> remove this. It may not be necessary. >>>>> >>>>> SWIG only installs the binary and .i files. My install-swig adds >>>>> the man page and also all the html and pdf docs which was >>>>> required by the ARC review. >>>>> >>>>> usr/src/pkgdefs/Makefile >>>>> >>>>> * put the changes on lines 283, 285-295 back (mismerge, same >>>>> as above) >>>>> Done >>>>> >>>>> usr/src/pkgdefs/SUNWswig/Makefile >>>>> usr/src/pkgdefs/SUNWswig/depend >>>>> >>>>> * If you only have the "standard" dependencies, use DATAFILE >>>>> in the >>>>> Makefile and don't deliver your own depend file. >>>>> Done >>>>> >>>>> Bruce >>>>> >>>>> Norm Jacobs wrote: >>>>> >>>>>> Here are some comments. >>>>>> >>>>>> -Norm >>>>>> >>>>>> >>>>>> usr/src/cmd/subversion/Makefile.sfw >>>>>> >>>>>> * remove line 35 >>>>>> * remove a ".WAIT" from line 36 >>>>>> * put lines 41, 60-61. 97-98, 134-135 back from the original >>>>>> (this >>>>>> appears to have been a mismerge and Mike will be unhappy if >>>>>> you >>>>>> undo his fix :-( ) >>>>>> * remove the SWIG related contents from lines 171-185, 189 >>>>>> >>>>>> usr/src/cmd/subversion/swig* >>>>>> >>>>>> * remove (use "wx rm ..." to do this) >>>>>> >>>>>> usr/src/cmd/swig/Makefile.sfw >>>>>> >>>>>> * line 33, why are you overriding "--with-swiglibdir" ? Is the >>>>>> default value somehow broken? >>>>>> * line 38, make sure that you are linking against the proto >>>>>> area, >>>>>> make sure you add --norunpath and check if you need the >>>>>> standard >>>>>> C++ libraries. >>>>>> * lines 38-41, add CFLAGS="$(CFLAGS)" to the environment. >>>>>> * line 45, remove >>>>>> * lines 48-52, remove >>>>>> * lines 56-57, add "INSTALL=$(INSTALL_PROTO)" and MANSCRIPT >>>>>> to your >>>>>> environment >>>>>> * lines 59-60, remove and use proto-fix to get the perms >>>>>> right on >>>>>> the proto area (see a2ps, hpijs, cups, or other component >>>>>> for an >>>>>> example) >>>>>> >>>>>> usr/src/cmd/swig/install-swig >>>>>> >>>>>> * incorporate the swig.1 manpage install into Makefile.sfw (and >>>>>> anything else that "cd $VER; make install" didn't cover) and >>>>>> remove this. It may not be necessary. >>>>>> >>>>>> usr/src/pkgdefs/Makefile >>>>>> >>>>>> * put the changes on lines 283, 285-295 back (mismerge, same >>>>>> as above) >>>>>> >>>>>> usr/src/pkgdefs/SUNWswig/Makefile >>>>>> usr/src/pkgdefs/SUNWswig/depend >>>>>> >>>>>> * If you only have the "standard" dependencies, use DATAFILE >>>>>> in the >>>>>> Makefile and don't deliver your own depend file. >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> Bruce Rothermal wrote: >>>>>> >>>>>>> Could someone please review swig changes. Changed subversion >>>>>>> Makefile to use new swig. >>>>>>> >>>>>>> http://cr.opensolaris.org/~bruce_r/swig/ >>>>>>> >>>>>>> >>>>> _______________________________________________ >>>>> 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 >>>> >> >> >> _______________________________________________ >> sfwnv-discuss mailing list >> sfwnv-discuss at opensolaris.org >> http://mail.opensolaris.org/mailman/listinfo/sfwnv-discuss >> > >
