On 04/24/08, Paul Cunningham wrote: > Raymond, > > Mainly looks good to me from my quick review. See minor comments below ... >
Paul, Thanks for your review. See my reply below... > Raymond Xiong wrote: > >Can anyone help to review my code for Erlang/OTP integration? > > > > http://cr.opensolaris.org/~rayx/erlang/webrev/ > > > >Here is its ARC material: > > > > > > http://opensolaris.org/os/community/arc/caselog/2008/210/materials/final-spec-txt/ > > > >The code is based on Rahul's original code for CCD integration, > >but I made some changes(Rahul and Jim has kindly reviewed some > >of those changes). > > > >Note: I knew of Targetdirs usage from recent discussion on this > >alias, and I have changed my code to use it. But I just use > >Targetdirs to create top-level directories and doesn' list all > >of subdirs(because there are too many), I suppose that's OK. > > > > === Start of Comments ========= > > 1. usr/src/cmd/erlang/Makefile.sfw > Could you have used the a default --prefix and --mandir are set in > Makefile.master ? See comment in > http://cr.opensolaris.org/~jacobs/2008-03-21-sfwnv-cups/usr/src/cmd/cups/Makefile.sfw.html Thanks for the pointer. Using default prefix value means using CONFIGURE_OPTIONS. If I modify my code to use CONFIGURE_OPTIONS, should I take the chance to use TARGET_ENV style coding in my Makefile.sfw too? What I mean is, the coding style of cups Makefile.sfw is quite different from most other exising applications on SFW gate. I can see the benefit of its apporach, but I am not sure what's the recommended way(since most others don't use that way). I wonder if we have a guideline for those details? > > 2. usr/src/pkgdefs/SUNWerlang-doc/depend > Does this really depend on SUNWerlang? After all its only > documentation which you could have installed without the > binary stuff! (Just a thought) It's strange that I didn't ever think it in that way. ;) Yes, you are right(I checked other documenation packages on SFW gate, they are all written in the way you suggested) I will remove SUNWerlang dependence from SUNWerlang-doc's depend file. > > 3. usr/src/cmd/erlang/postinstall-sfw > Saw your comment above - but I would say the dirs should be in > Targetdirs. Below is the new top-level directories created by postinstall-sfw script: /usr/share/man/man1erl /usr/share/man/man3erl /usr/share/man/man4erl /usr/share/doc/erlang All of them are defined in Targetdirs. /usr/share/doc/erlang has three subdirs: doc, erts-5.6.1, and lib. These three subdirs has many subdirs, which in turn have their own subdirs. The total subdir number is 172. My question is: should I list all of them in Targetdirs(I doubt it) or is it OK to just add the following(currently I only add /usr/share/doc/erlang, the top-level dir, in Targetdirs): /usr/share/doc/erlang |- /usr/share/doc/erlang/doc |- /usr/share/doc/erlang/erts-5.6.1 +- /usr/share/doc/erlang/lib Note that while I don't define those subdirs in Targetdirs, I don't create them explicitly in postinstall.ksh. Instead, they are created implicitly by "cp -r". BTW, what's the purpose of Targetdirs? Is it only used as a conveninet way to create directories, or is it also used to track *every* new subdirs on the system? > 4. usr/src/pkgdefs/SUNW*/copyright > Just my normal comment about the length of the 'copyright' files :-( > Have noticed lately, in reviews, that some people are using > the short version. (Ignore if you so wish) I saw the discussion, though I am not sure what's the final dicission(I think the prefered way is to incude the whole license?). What the short version is like? Is it just a URL? (BTW, this is EPL, not GPL) Thanks, Raymond
