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/"; 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 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"; 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"; 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. 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' 9. usr/src/pkgdefs/SUNWjedit/pkginfo.tmpl Change the DESC= line so the version is in brackets, eg .. DESC="........... (4.3pre16)" 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. 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"; === End of Comments ===== -- ---------------------------------------------------------------------- Paul Cunningham Software Engineer Tadpole Business Unit
