Raymond, See below ...
Paul Raymond Xiong wrote: > On 04/24/08, Paul Cunningham wrote: >> >>Mainly looks good to me from my quick review. See minor comments below ... > > 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? Sorry I don't know. It was just a thought anyway so I am happy if you ignore this comment. >>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. I think (if I remember correctly) that your 'depend' would then be the same as the default one - so you could remove yours and change the pkg's Makefile to use the default one. >>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? I seem to remember (from my time at Sun) it used to be a big no-no not to have them in the sfw Targetdirs. But if the current gatekeeper is happy for it to be done the way you have done it, it's also okay with me :-) >>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) I seem to remember that another review, sent out in the last few days, used the short version (so you could track that down and look at that as an example if you want one) -- ---------------------------------------------------------------------- Paul Cunningham Software Engineer Tadpole Computer Products
