Hi Paul, Thanks for your review, I have made some changes according to your suggestion, please see my inline comments below:
Regards, Rusong Paul Cunningham ??: > Rusong, > > See below for my comments ... > > Paul > > Rusong.Zheng wrote: >> >> I'm porting jedit, a programmer's text editor written in Java. >> >> Please help to code review my modification. The webrev is at: >> >> http://cr.opensolaris.org/~rszheng/jedit/ >> >> Any comments are welcomed. The comments will be changed accordingly once >> the subcategory gets approval. Thanks. > > === Start of Comments === > > 1. usr/src/cmd/jedit/METADATA > Add the missing fields, see ... > "http://wikis.sun.com/display/SFWNotes/Package+writing+guidelines"; > > 2. CDDL HEADER and top of file (various files > Change the top of files so that they conform to the > prototypes in ... > "http://src.opensolaris.org/source/xref/onnv/onnv-gate/usr/src/prototypes/"; > I have changed these files and code section accordingly. > > 3. usr/src/cmd/jedit/Makefile.sfw > Extract the JEDIT= info from the METADATA. > Also add TARBALL= and replace '$(JEDIT)source.tar.bz2' > throughout with $(TARBALL). > see example in .. > "http://src.opensolaris.org/source/xref/sfw/usr/src/cmd/cups/Makefile.sfw"; > > > Lines ... > 34 cp $(JEDIT_SRC_DIR)/build.properties.sample > $(JEDIT_SRC_DIR)/build.properties; > 35 $(GPATCH) -p0 < build.properties.patch; > 36 $(GPATCH) -p0 < build.xml.patch; > it might be better if these were moved into the > $(JEDIT_SRC_DIR): rule I have moved decompression and patch to $(JEDIT_SRC_DIR) rule, and also add a patch--jedit.patch, which is used to generate a script in /usr/bin to invoke jedit in CLI. I also added two new variables PROG_NAME and PROG_VER to hold jedit name and version, which will be used in install.sfw. > > 4. usr/src/cmd/jedit/install-sfw > Line ... > 27 VER="4.3pre16" > pass this info in from the Makefile.sfw where it is > extracted from the METADATA > > Change as .. > Roland Mainz wrote: > > add a $ set -o errexit # at the beginning > as in ... > "http://src.opensolaris.org/source/xref/sfw/usr/src/cmd/meld/install-sfw"; I add BINDIR=/usr/bin, where the executable script file for jedit is installed and changed JEDIT_ROOT to JEDIT_ROOT=${ROOT}/usr/share/${PROG_NAME}/${PROG_VER} > > 5. usr/src/pkgdefs/SUNWjedit/copyright > Add the source owner copyright lines extracted from > the src files in the unpacked tarball, as in ... > "http://src.opensolaris.org/source/xref/sfw/usr/src/pkgdefs/SUNWmeld/copyright"; > > The missing part was added. > > 6. usr/src/pkgdefs/SUNWjedit/Makefile > You have your own 'depend' file so delete line ... > 30 DATAFILES= depend > unless you have no other dependencies (see item 7) in > which case remove your 'depend' from the webrev. > > 7. usr/src/pkgdefs/SUNWjedit/depend > Move the Copyright lines to after the > "CDDL HEADER END" header > > Correct Copyright year. > > Are you sure you have no other dependencies, shouldn't > it have a dependency on the java runtime pkg? > If none - then see item 6. I kept the DATAFILES=depend and deleted depend file for that the installation of jedit does not depend on other packages. Though the jre is needed to run jedit ( but for solaris/opensolaris, the jre is always installed with the fresh system) > > 8. usr/src/pkgdefs/SUNWjedit/prototype_com > How do I invoke the jedit executable? shouldn't there > be something in /usr/bin ? > The manual .. > "http://www.jedit.org/users-guide/starting.html"; > says ... > 'on Unix you can run ?jedit? at the command line' > A new script to call java -jar jedit.jar is added , so $ jedit filename1 ... will be worked in CLI, see comment for item 3 above. > 9. usr/src/pkgdefs/SUNWjedit/pkginfo.tmpl > Change the DESC= line so the version is in brackets, eg .. > DESC="........... (4.3pre16)" Modified as your suggestion. > > 10. usr/src/pkgdefs/SUNWjedit/prototype_com > There is rather a lot of html documentation in here, have > you thought about splitting it out into its own SUNW pkg, > eg. SUNWjedit-doc maybe. jEdit is a GUI editor integrated with help files, API and manual docs. Installing binary file and doc files together at one time maybe straightforward. And also these html files are compiled from docbook-style files which are contained in source code of jedit. It maybe more easier to build and install if putting them together. > > 11. usr/src/pkgdefs/SUNWjedit/prototype_sparc > & usr/src/pkgdefs/SUNWjedit/prototype_i386 > Add the package name on a comment line as in .. > "http://src.opensolaris.org/source/xref/sfw/usr/src/pkgdefs/SUNWmeld/prototype_i386"; > > Modified as your suggestion. > > === End of Comments =====
