Hi Vladimir, Thanks for the review. Please find comments below.
On Wed, Dec 23, 2009 at 01:30:38PM +0100, Vladimir Marek wrote: > Hi, > > > > Please code review fix for following CRs: > > > > 6825333 Update fetchmail to version 6.3.13 > > 6802880 Add NTLM support to fetchmail > > > > The webrev is available at: > > http://cr.opensolaris.org/~aragorn/6825333-fetchmail/ > > The change looks mostly good to me, apart from the > > > > 6. Added 'env -' for all relevant targets. > > 58 - (cd $(PROD); $(CCSMAKE) MAKE=$(CCSMAKE)) > 63 + (cd $(PROD); env - $(CCSMAKE)) > > 61 - (cd $(PROD); $(CCSMAKE) install \ > 62 - MAKE=$(CCSMAKE) \ > 63 - DESTDIR="$(ROOT)" \ > 64 - INSTALL=/usr/bin/ginstall \ > 65 - pythondir="$(PYTHONDIR)") > 66 - $(RM) $(ROOT)/usr/bin/fetchmailconf > 66 + (cd $(PROD); env - $(CCSMAKE) install \ > 67 + DESTDIR="$(ROOT)") > > make does not use environment variables unless told by -e (apart from > things like MAKEFLAGS afaik). So env - here buys us nothing. Correct me > please if I'm wrong. Sorry, that's not true: $ echo $MAKEFLAGS $ cat Makefile all: @echo $(TERM) $ /usr/ccs/bin/make xterm $ TERM=a /usr/ccs/bin/make a $ uname -a SunOS telcontar 5.11 snv_130 i86pc i386 i86pc $ -e is related, but used for something a bit different: -e Environment variables override assignments within makefiles. > > > 84 -$(PROD)/config.status: $(PROD)/patched > 85 - (cd $(PROD); ./configure \ > 86 - --prefix=/usr \ > 87 - --with-kerberos5 \ > 88 - --with-ssl \ > 77 +$(PROD)/config.status: $(PROD)/configure > 78 + (cd $(PROD); env - ./configure \ > 79 + $(CONFIGURE_OPTIONS) \ > > Here it is questionable, as configure uses environment variables. But > then if build environment exports some variables, is here the correct > place to fix it? Where I can see advantage, if someone tries to build > fetchmail out of build environment (nightly), and he has some special > environment set in his shell. In this case we would protect him from > himself. But should we? Definitively yes. "env -" is currently recommended practice in sfwnv. Without "env -" we saw several times that build environment unexpectedly set some env. variables and the build did not worked as expected. Sorry, I do not have any example handy, but when needed I'll try to find some examples in the mailing list archives or bugster. Please let me know... :-) If somebody wants to build fetchmail himself with his options he could do that using ./configure --WHATEWER; make; make install > > > Not that these would cause any issues, I just wonder if it's some sort > of recommendation to do that. > > > > > 14. shebang for /usr/lib/python2.6/vendor-packages/fetchmailconf.py is > > modified > > to use explicit version of python (2.6) - please see the PERL line in the > > Makefile.sfw. > > 69 + $(PERL) -p -i -e 's,^#!/usr/bin/env python$$,#!/usr/bin/env > $(PYTHONBIN),' $(ROOT)$(PYTHONDIR)/fetchmailconf.py > > so the resulting line will be "#!/usr/bin/env /usr/bin/python2.6" > If you use full path of python2.6, it IMO defeats purpose of > /usr/bin/env. Would not it be sufficient to use just > > "#!/usr/bin/python2.6" ? You are true. I'll update the webrev soon. > > And just for sake of visual pleasure-ness, the line 69 could be split in > two. Ok. Valid nit. I'll make sure to not have so long lines. Thank you. -- Marcel Telka Solaris RPE
