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. 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? 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" ? And just for sake of visual pleasure-ness, the line 69 could be split in two. -- Vlad -------------- next part -------------- A non-text attachment was scrubbed... Name: not available Type: application/pgp-signature Size: 193 bytes Desc: not available URL: <http://mail.opensolaris.org/pipermail/sfwnv-discuss/attachments/20091223/09daeca5/attachment.bin>
