Charles Baker wrote: > A code review for the proposed inclusion of axyftp into the > sfw consolidation has been posted to: > > http://cr.opensolaris.org/~cebaker/axyftp-6674220/
Quick 5min race through http://cr.opensolaris.org/~cebaker/axyftp-6674220/sfwnv-clone.patch (patch code is quoted with "> "): > --- /dev/null Tue Apr 15 15:24:17 2008 > +++ new/usr/src/cmd/axyftp/Makefile.sfw Tue Apr 15 15:24:17 2008 > @@ -0,0 +1,64 @@ [snip] > + > +VER=axyftp-0.5.1 > + > +include ../Makefile.cmd > + > +all: $(VER)/config.status > + (cd $(VER); env \ > + LD_OPTIONS="-M $(SRC)/cmd/mapfile_noexstk" \ > + CC=$(CC) \ > + "CFLAGS=$(CFLAGS) -H" \ 1. Please add "-xc99=%all -D_XOPEN_SOURCE=600 -D__EXTENSIONS__=1" to CFLAGS (to get the compiler behave more like gcc (C99/XPG6-like) and as a nice bonus system calls like |popen()|, |system()| etc. will use the POSIX shell instead of the Bourne shell (avoiding some complains in that area)) and "-xstrconst" (which puts string literals into the code section and therefore saves some memory) to these flags (if you hit any problems please email me via PM), e.g. the line should be... -- snip -- CFLAGS += -xc99=%all -D_XOPEN_SOURCE=600 -D__EXTENSIONS__=1 -xstrconst -xO3 all: $(VER)/config.status (cd $(VER); env \ LD_OPTIONS="-M $(SRC)/cmd/mapfile_noexstk" \ CC=$(CC) \ "CFLAGS=$(CFLAGS) -H" \ PATH=$(PATH) \ MAKE=$(MAKE) \ $(MAKE) -e) @find . -name core -exec rm -f {} \; -- snip -- (... and please check the build logs whether "configure" got the same flags) 2. Why do you use "-H" in this case (look like a debug option, right ?) ? > + PATH=$(PATH) \ > + MAKE=$(MAKE) \ > + $(MAKE) -e) > + @find . -name core -exec rm -f {} \; > + > +install: all > + VERS=$(VER) $(SH) ./install-axyftp > + > +$(VER)/config.status: $(VER)/configure > + (cd $(VER); env \ > + CC=$(CC) \ > + "CFLAGS=$(CFLAGS) -H" \ See comment about "-H" above... > + CONFIG_SHELL=/bin/sh \ Groan... I suggest to use /usr/xpg4/bin/sh (the POSIX shell) instead of /bin/sh - it is much closer to the user's "expectations" than /bin/sh (BTW: If you want to use "/bin/sh" use "/usr/bin/sh" - "/bin" is a softlink to "/usr/bin" ...). The remaining stuff looks AFAIK good... ---- Bye, Roland -- __ . . __ (o.\ \/ /.o) roland.mainz at nrubsig.org \__\/\/__/ MPEG specialist, C&&JAVA&&Sun&&Unix programmer /O /==\ O\ TEL +49 641 7950090 (;O/ \/ \O;)
