Hi Spoorthy,
Below are my comments:
1. In METADATA file,
- Please remove below line.
11 PSARC:
- Add BUGTRAQ field.
2. Please add the new product's entry in usr/src/cmd/Makefile under
'COMMON_SUBDIRS' in alphabetical order. This is to ensure that nightly
script builds the new component from the toplevel dir.
3. In Makefile.sfw,
- Instead of using two command 'gzip' and 'tar' you could use gtar with
'-z' option. Something like
$(GTAR) xzpf $(TARBALL) --no-same-owner;
- What CFLAGS and CXXFLAGS are you using while building?
- In install target, you could also pass on the VER info as below:
PKGVERS=$(VER) $(SHELL) ./install-sfw
With this you could remove the hard coding of version information done
in install-sfw script.
4. usr/src/cmd/conman/conman.1 and usr/src/cmd/conman/conman.conf
- Are these Man pages already available in the source tarball or are
these newly written man pages ?
If they are newly written man pages, then they need to have CDDL
headers, Sun Copyright and the sunman-stability.
If they are available along with source tarball then there is no
requirement to check them into the workspace. Instead you could just
install them from the build dir after adding the sunman-stability.
5. usr/src/cmd/conman/conmand.xml
- The CDDL headers are not in proper format. Please check other manifest
files under '/var/svc/manifest' dir. You could also refer to recent
integration/reviews of pen or openwsmand.
- In line 43, the package name should be SUNWconmanr as below:
43 <service_bundle type='manifest' name='*SUNWconmanr*:conmand'>
- Manifest file doesn't seem to have the exec_method to stop/disable the
service. Is this intended ? If not, a new exec method to stop the
service needs to be added.
- Also curious to know if this service can be managed by any normal user
or does it require any special privileges ?
6. In usr/src/cmd/conman/install-sfw,
- Please change
1 #! /usr/bin/bash
to
1 #!/bin/ksh93
- As mentioned in comment (3) you could get the PKGVERS info from
Makefile.sfw
- In line 54, svc-conmand is a script and should be installed with 'S'
option and not as a Normal file.
- From lines 57-60, do you really require 744 permissions ? If not
please change the same.
- In lines 51, 57, 58 and 60, you are installing the same file into
different locations. Is this intended ?
- All files specified from lines 62 to 82 are installed as Manpages
using "_install M". Please use 'N' if they are normal files.
7. Please add entries for the new packages SUNWconmanu and SUNWconmanr
in usr/src/pkgdefs/Makefile. This change is currently not present in the
webrev.
8. usr/src/pkgdefs/SUNWconmanr/,Makefile should be removed. The ,xyz
file gets created when you create a new file using sccs. You should not
be including it in webrev.
9. usr/src/pkgdefs/SUNWconmanr/Makefile
- There is no 'ident' string. Please add it.
- The depend file seems to be the default one. So you could remove the
depend file for SUNWconmanr and add "DATAFILES = depend" entry in the
Makefile.
- Remove the extra '#' from line 33.
10. usr/src/pkgdefs/SUNWconmanr/copyright and
usr/src/pkgdefs/SUNWconmanu/copyright
- Source owner copyright lines are missing.
11. Please remove the file usr/src/pkgdefs/SUNWconmanr/pkginfo from the
ws active list as it gets generated from
usr/src/pkgdefs/SUNWconmanr/pkginfo.tmpl.
12. In usr/src/pkgdefs/SUNWconmanr/pkginfo.tmpl, please replace
46 CLASSES="manifest"
with
46 CLASSES="none manifest"
Also please remove the extra line from the end of the file.
12. usr/src/pkgdefs/SUNWconmanu/Makefile
- Please add the 'ident' string to the Makefile.
- Are there no other dependencies other than SUNWconmanr? Please check
by running the dependency checker script.
13. Please remove the file usr/src/pkgdefs/SUNWconmanu/pkginfo.
14. In usr/src/pkgdefs/SUNWconmanu/pkginfo.tmpl, please replace
46 CLASSES="manifest"
with
46 CLASSES="none"
Also please remove the extra line from the end of the file.
15. usr/src/pkgdefs/SUNWconmanu/prototype_com
- In line 49, you seem to be installing 'conmen'. This isn't present in
install-sfw and hence will also be not present in the proto area. So
packaging would fail for the same reason. Please check.
49 f none usr/bin/conmen 0555 root bin
16. In almost all the files, CDDL header requires cosmetic changes.
Please follow the format for the CDDL headers and Sun Copyright lines as
specified in the link
http://src.opensolaris.org/source/xref/onnv/onnv-gate/usr/src/prototypes/
17. Please make sure all the files in your webrev has comments of the
format "<bugid> <synopsis>"
18. Please ensure that you run a clean nightly build of your workspace
before regenerating the webrev and make sure there are no errors in the
nightly build log.
Thanks,
Srirama
Spoorthy H.S said the following on Monday 16 March 2009 03:18 PM:
> Hi All,
>
> Conman is the Console Management tool. More info on the tool can be
> found at http://home.gna.org/conman/
> Here's the webrev .. http://cr.opensolaris.org/~spoorthy/conman22/
>
> Thanks
> Spoorthy
>
> _______________________________________________
> sfwnv-discuss mailing list
> sfwnv-discuss at opensolaris.org
> http://mail.opensolaris.org/mailman/listinfo/sfwnv-discuss
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://mail.opensolaris.org/pipermail/sfwnv-discuss/attachments/20090316/ffd57efa/attachment.html>
