Hi Enrique,
Actually that was one of the questions. If there are not dependencies for
the package do we still need to add "depend" file?
I have just replied to Paul's email on point number 8 he mentioned which is
as below.
8. Dependencies
You are using the default 'depend', have you checked there
are no other dependencies?
[VIVEK] No other dependencies so default is 'depend'.
Pl. suggest.
Thanks,
~Vivek R. Titarmare
From: Enrique.Lopezpineda at Sun.COM [mailto:[email protected]]
Sent: Wednesday, March 11, 2009 8:51 PM
To: Vivek Titarmare
Cc: Muktha.Narayan at Sun.COM; 'Paul Cunningham'; sfwnv-discuss at
opensolaris.org
Subject: Re: [sfwnv-discuss] Request code review for "antlr277"
Vivek,
I still don't see a "depend" file; shouldn't that file be part of the
package?
Also, Paul, the package to review is antlr277 (vs Git - which was
integrated last year, and
for which you also provided a review - thanks!).
Enrique
Vivek Titarmare wrote:
Hi,
As mentioned below almost all the points are updated and the new webrev is
uploaded. The url is http://cr.opensolaris.org/~vivekrt/antlr277.
Couple of points to note is as follows:
1. Point Number 11 could not be completed as we are working on categories
and sub-categories for the bugID to be filed. So I have mentioned Pending in
METADATA file.
2. Secondly the build.xml is completely removed and the code is tested
after removing.
Let me know if I have missed anything.
Thanks,
~Vivek R. T.
-----Original Message-----
From: Muktha.Narayan at Sun.COM [mailto:[email protected]]
Sent: Tuesday, March 10, 2009 5:55 PM
To: Vivek Titarmare
Cc: sfwnv-discuss at opensolaris.org
Subject: Re: [sfwnv-discuss] Request code review for "antlr277"
Hi Vivek,
Below are some comments on the antlr webrev at
http://cr.opensolaris.org/~vivekrt/webrev.
Also note that some of the comments may be same as what Paul has already
mentioned.
1. Targetdirs
a) Please include the entries in alphabetical order.
2. METADATA
a) There are few fields missing in METADATA (NAME, SRC, SUPPORT,
BUGTRAQ), please follow the rules specified in the link below :
http://wikis.sun.com/display/SFWNotes/Package+writing+guidelines
b) Also, you should specify your mail id in OWNER section.
3. usr/src/lib/Makefile
a) Please include the entries in alphabetical order.
4. Please check CDDL Header for all the files. It should follow
guidelines specified at below location:
http://src.opensolaris.org/source/xref/onnv/onnv-gate/usr/src/prototypes/
5. In Makefile.sfw,
a) ANTLR information could be obtained from the METADATA file, instead
of hard coding it in the Makefile.sfw.
something like:
ANTLR= $(COMPONENT_NAME:sh)-$(COMPONENT_VERSION:sh)
b) Please do not use "pragma ident" keywords in files other than source
files. In install scripts, Makefiles and other files you should be using
"ident" keyword.
c) Are you using the default Java version ? I don't see any specific env
being set before building using ant. Is this intended ?
6. Please make sure the Sun Copyright year is set to 2009 in all files.
7. In install-sfw,
a) It is advisable to get the VER info from Makefile.sfw instead of
specifying it again in install-sfw file.
Something like below can be done in your Makefile.sfw:
VER=$(ANTLR) $(SHELL) ./install-sfw
b) You could add below lines to make sure the script stops as soon as it
encounters first error
# stop at first error
set -o errexit
c) It is not required to specify the permission while installing
symbolic links
72 _install L ../../lib/java/javadoc/antlr277 ${DOCDIR}/javadoc 444
8. In the manpage antlr277.3, there is a small typo. A missing space
(forconstructing)
9. build.xml
a) Is there any reason why the default Makefile cannot be used which is
available in the source tarball.
b) Also, please check if this file needs to have CDDL header.
10. prototype_com
a) shouldn't the below be antlr277.3 ?
54 f none usr/share/man/man3/antlr.3 0444 root bin
b) Is the below entry required?
57 d none usr/share/doc/junit 0755 root other
11. Please make sure that the comments for all the files listed in
webrev are of the format "<bugid> <synopsis>"
12. The copyright file should have the source owner copyright lines
followed by the 'BSD license'.
Regards
Muktha
Paul Cunningham wrote:
Vivek,
See below for my comments ...
Paul
Vivek Titarmare wrote:
I have posted a webrev for the package "antlr277" which I am porting
to OpenSolaris and would
like to request a code review.
Please see http://cr.opensolaris.org/~vivekrt/webrev/
<http://cr.opensolaris.org/%7Eelpineda/git/webrev/>
<http://cr.opensolaris.org/%7Eelpineda/git/webrev/> and provide any
comments as
needed if there are any issues which I need to correct.
Have I reviewed the right thing here "antlr277" v "git" ?
=== Start of Comments ===
1. usr/src/cmd/Makefile
I am guess that this is out of sync with the gate as the
copyright year is wrong - your ws may need resyncing
2. usr/src/cmd/git/METADATA
Please change so that the content conforms to that in ..
<http://wikis.sun.com/display/SFWNotes/Package+writing+guidelines>
"http://wikis.sun.com/display/SFWNotes/Package+writing+guidelines";
3. CDDL HEADER and top of files (various files)
Cosmetic: Please change the files so that the tops
conform to those in ...
<http://src.opensolaris.org/source/xref/onnv/onnv-gate/usr/src/prototypes/>
"http://src.opensolaris.org/source/xref/onnv/onnv-gate/usr/src/prototypes/";
4. Copyright year (various files)
Please correct the year on the file Copyright line
5. usr/src/cmd/git/Makefile.sfw
Extract the VER= info from the METADATA, see recent sfw
integrations for examples. And also for MANPAGES=
Change so that the pkg 'configure' is done a separate
rule ($(VER)/config.status: is normal)
Change the pkg configure so it is done using the
CONFIGURE_OPTIONS method, eg, something like ...
./configure $(CONFIGURE_OPTIONS)
as you have no additional options other than those already
predefined in the Makefile.master
Lines 48-52, could you have done this with a patch file
instead on the pkg's Makefile.in prior to the configure?
Lines 64 & 71, do you need the 'prefix=$(PREFIX)'
on this 'makes' as you have specified it already
on the configure?
Lines 87 to 89, why the multiply 'rmdir's, couldn't you
have just done '/usr/bin/rm -rf tmp' after line 89 ?
Line ...
65 @find . -name core -exec rm -f {} \;
do you need this, if so why?
6. usr/src/pkgdefs/Makefile
It might be better if SUNWgit was moved to after
SUNWgimpprint so its more alphabetical
7. usr/src/pkgdefs/SUNWgit/copyright
Add the pkg source owner copyright lines after the
sun disclaimer, ie. those extracted from the files in
the unpacked src tarball, something like this ...
<http://src.opensolaris.org/source/xref/sfw/usr/src/pkgdefs/SUNWmeld/copyrig
ht>
"http://src.opensolaris.org/source/xref/sfw/usr/src/pkgdefs/SUNWmeld/copyrig
ht
<http://src.opensolaris.org/source/xref/sfw/usr/src/pkgdefs/SUNWmeld/copyrig
ht> "
8. usr/src/pkgdefs/SUNWgit/prototype_i386
& usr/src/pkgdefs/SUNWgit/prototype_sparc
Are the permissions correct on the installed files (0400)?
=== End of Comments =====
--
Enrique Lopez-Pineda
Sun Microsystems
enrique.lopezpineda at sun.com
651-552-6426 (x44626)
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://mail.opensolaris.org/pipermail/sfwnv-discuss/attachments/20090312/54857453/attachment.html>
