1. usr/src/cmd/proftpd/Makefile.sfw - for the $(VER)/configure target, best to stick with what everyone else does:
gzip -dc $(VER).tgz | tar xopf - and never use absolute paths to commands (i.e. /usr/sfw/bin/gtar) - are the LD_OPTIONS lines required? The mapfile is already specified in the SFW LDFLAGS and usr/lib shouldn't need to be called out explicitly for the Link time/runtime path. - There's no C+_ code is there? So there should be no need to set CXX - Is the find command necessary? Do you expect that the build will generate core files? I know that it happens in some cases but often it seems to be a case of defensive programming. - Why do you set a variable called VERS in the environment for the configure script? 2. usr/src/cmd/proftpd/METADATA - I'd suggest that you remove the comment and if there are not other comments just leave it blank 3. usr/src/pkgdefs/SUNWproftpdu/prototype_com - I couldn't see any reason to have the following: d none usr/libexec 755 root root You don't install files into it, but maybe it's required at runtime. Can you clarify. Other than the above comments everything else looks ok. Amanda Jerry Jiang wrote: > Hi all, > > I will update the source files of proftpd 1.3.1 according Muktha and > Amanda's comments. The new webrev has been posted to: > > http://cr.opensolaris.org/~bgun/proftpd/ > > Please help me review it again and send me your comments. > > Thanks, > Jerry > > > Amanda's comments: > > *You need to resync with the parent as you are removing all of the > stuff added to SFW in the last few weeks. Look at: > > usr/src/Targetdirs > usr/src/Makefile > usr/src/pkgdefs/Makefile > > All of the lines in red are in the parent workspace but not in yours > so you are effectively removing them. > * > Yes, I have resync my workspace with the parent this time. > > > Mujtha's comments: > > */Below are my comments. > 1. METADATA file is missing. > Please include the file and follow the guidelines outlined at > http://wikis.sun.com/display/SFWNotes/Package+writing+guidelines > > /*Yes, METADATA file is added. > */ > 2. Please change the copyright year to 2009 through out. > > /*Yes, the copyright year is 2009 now. > > */3. Makefile.sfw > a) You can avoid hard-coding program name and version and instead > extract this info from METADATA, something like: > VER = $(COMPONENT_NAME:sh)-$(COMPONENT_VERSION:sh) > b) Use env - instead of env through out. > c) It is advisable to use $(SHELL) instead of $(SH). > d) You may want to call configure as "$(SHELL) ./configure" > e) You could use > CONFIGURE_OPTIONS += --localstatedir=/var --sysconfdir=/etc > and then call configure as $(SHELL) ./configure $(CONFIGURE_OPTIONS) > f) If you intend to use /usr/sfw/bin/gtar then you could use it with > "-z" option to unzip the tarball instead of doing the same using 2 > commands (/usr/bin/gzip -dc and /usr/sfw/bin/gtar ) > > /*Yes. All these items are changed. > > */4. install-sfw > a) It is recommended to use /usr/bin/ksh93 instead of /bin/sh. This > allows you to add: > # stop at first error > set -o errexit > And to call "source" instead of "." in this line: > source ${SRC}/tools/install.subr > b) You can pass VERS as an environment variable from the Makefile and > use it here. > c) You can use SBINDIR in the below: > > _install L proftpd ${ROOT}/usr/sbin/in.proftpd > > d) You could have a for loop to install the header files. > e) Do you require the write permission on the header files and man pages? > > /*Yes. All these items are changed. > > */5. SUNWproftpdr/Makefile > a) You could use the default depend file here by adding > "DATAFILES=depend" in the Makefile if there are no project specific > dependencies. Please check. > > /*Yes. This item is added and file "depend" was deleted. > > */6. SUNWproftpdr/pkginfo.tmpl > a) You may want to change the DESC field as proftpd - Highly > configurable FTP daemon server (1.3.1) (Root) > > /*Yes. It was changed. > > */7. SUNWproftpdr/prototype_com > a) Are these two entries required? 48 d none var 755 root sys > 49 d none var/proftpd 755 root root > /* > Yes. Theses two entries are required for log file of proftpd. > */ > 8. SUNWproftpdu/depend > a) Move the copyright line after the CDDL header end. > b) Shouldn't SUNWproftpdu depend on SUNWproftpdr? If so, the same > needs to be mentioned in the depend file. > > /*Yes. All these items are changed. > > */9. SUNWproftpdu/pkginfo.tmpl > a) You may want to change the DESC field as proftpd - Highly > configurable FTP daemon server (1.3.1) (Usr) > /* > Yes. It was changed. > > */10. SUNWproftpdu/prototype_com > a) Is the below entry required? d none usr/libexec 755 root root > > /*Yes. It was needed for lib files. > > */11. You seem to be removing few entries from cmd/Makefile, > Targetdirs and pkgdefs/Makefile files. Please check. > > /* Yes. I have check all these files. > >
