Date: Tue, 26 Jan 2010 10:50:54 -0800 From: Doug Barton <dougb@FreeBSD.org> To: =?ISO-8859-1?Q?Francisco_de_Borja_L=F3pez_R=EDo?= <borja@pexego.es> Cc: ports@freebsd.org Subject: Re: New port: finance/openerp-web Message-ID: <4B5F398E.5060500@FreeBSD.org> In-Reply-To: <20100126174903.b144472f.borja@pexego.es> References: <20100120165147.666592ef.borja@pexego.es> <20100120212640.GE6618@lonesome.com> <7be7a2801001201600j5c772267na6dea944334618be@mail.gmail.com> <20100121093649.7867d810.borja@pexego.es> <20100121095114.12bab6af.borja@pexego.es> <4B5BEDB1.8080308@FreeBSD.org> <20100126174903.b144472f.borja@pexego.es>
next in thread | previous in thread | raw e-mail | index | archive | help
On 01/26/10 08:49, Francisco de Borja López Río wrote:
> On Sat, 23 Jan 2010 22:50:25 -0800
> Doug Barton<dougb@freebsd.org> wrote:
>
>> On 01/21/10 00:51, Francisco de Borja López Río wrote:
>>>
>>> Sorry, I forgot to add the attachments in my previous email.
>>>
>>> These are both the rc.d script and the default configuration file I've
>>> been using with the current openerp-server port for some time.
>>
>> A few notes on the rc.d script.
>>
>> 1. The name of the file, PROVIDE, and $name should all match. Perhaps
>> openerpd, or even erpd?
>
> If there is no limitation on that, it should be
> openerp-web/openerp-server (for the web and server scripts
> respectively). Let me try if it works with the -
I don't think it will work with the -, and generally simpler/shorter is
better. I personally would rather see just openerp and openerpd, but I
won't object to whatever you come up with as long as the 3 things are
the same for each of the scripts.
>> 2. There are quite a few examples of "/usr/local" in there (which is
>> fine for a running script), please remember to s#/usr/local#%%PREFIX%%#g
>> for the port.
>
> mmm, I've been taking a look at the rc.d scripts in my servers, and all
> of them have those absolute paths.
>
> Did you mean to set %%PREFIX%% in the rc file, or did you mean to
> generate the rc.d script dinamically when installing the port?
Look at the files in the ports tree for those and you should see what I
mean. Also see the porter's handbook link I posted regarding how to use
rc.d stuff in your port.
>> 3. The pidfile= should come after load_rc_config, and should look like
>> this:
>> pidfile="${<name>_pidfile:-/var/run/openerp-server/openerp-server.pid}"
>> (substitute<name> of course). Or, consider not allowing the user to
>> change the location of the pidfile unless there is a good reason to do so.
>
> mmm I do something similar to that,
Yes, I saw what you did. :) It's overly complicated and IIRC won't
actually work if the user tries to set ${name}_pidfile in rc.conf. OTOH,
what I suggested will definitely work, and is simpler.
Generally however it's not necessary to allow the user to change the
location of the pidfile unless there is a compelling reason to do so.
> I don't remember exactly where (probably somewhere in the porters
> handbook), but I've read that the order is not a problem (using
> openerpserver_pidfile before setting it) (IIRC)
If you can find the reference please report it, since it's not accurate.
You cannot access the values of any of the ${name}_* variables until
after load_rc_config.
>> 4. I'm not quite sure what eval "${rcvar}=\${${rcvar}:-'NO'}" is
>> supposed to accomplish, but if it's "set ${name}_enable to NO by
>> default" it's too clever by half. :) Take a look at
>> http://www.freebsd.org/doc/en_US.ISO8859-1/books/porters-handbook/rc-scripts.html
>> for other ways to simplify the default variable assignments (as well as
>> some other pointers).
>
> I got that from this article:
>
> http://www.freebsd.org/doc/en_US.ISO8859-1/articles/rc-scripting/rcng-confdummy.html
>
> (check point 3) Perhaps it is not needed tough.
Ok, I'll add that to the list of doc bugs. :) Much better (in the sense
of easier to read, easier to maintain, etc.) to just use the default
variable assignment method in the porter's handbook.
>> 5. Just doing 'touch foo' is always cheaper than testing its existence
>> first (although it's a micro-optimization) and there is no reason to
>> test existence anyway. Same goes with the 'mkdir -p'.
>
> mmm, you are right, but if you try to create a directory that already
> exist, a message about the already-existing directory will appear
> (blabla File exists).
You really need to test the things I suggested, and preferably read the
man pages to educate yourself about these topics. (Hint: I said 'mkdir
-p', not 'mkdir')
>> 6. Instead of `dirname blah` you should use ${blah%/*} for two reasons,
>> first dirname(1) is in /usr/bin, and may not be available at boot, and
>> two there is no reason to use it in a shell script when the variable
>> substitution is cheaper and easier.
>
> I got the dirname idea from the milter-bogom port rc.d script. It
> seemed reasonable for me ;).
Ok, thanks, I'll run a check for that too when I have more time. One of
the reasons that I am so pedantic about doing the rc.d scripts right the
first time is that bad code gets copied over and over again.
> Anyway, with variable substitution you meant, for example, setting a
> variable for the directory where the pidfile will be saved, then use
> that variable when creating the pidfile variable?
Nope. Test what I wrote.
blah=/one/two/three
echo ${blah%/*}
This one is what I would consider an "intermediate" level shell
scripting technique, which is why I am spelling it out for you in more
detail, but seriously, when someone gives you suggestions, try them.
It's the easiest way to learn. :)
>> 7. You named your substitute stop_cmd "${name}_poststop" which is
>> confusing, and I'm not sure why it's necessary. If you're afraid that
>> rc.d isn't going to actually stop the process for some reason we should
>> address that. If you feel that you absolutely must have a safety belt
>> for this make it a real stop_postcmd.
>
> Typo, it should be _stop (will be fixed in just a matter of minutes :D,
> thnx for the report)
I'm glad you caught the typo, but you still haven't answered my
question. Why do you need to redefine the _stop method?
>> I realize that these are a lot of notes, but don't worry ... it's easy
>> to see that a lot of thought and creativity went into this, which is why
>> I'm taking the time to try and help improve it.
>
> That's exactly what I want, I'm working in more scripts and stuff, so
> learning some basic stuff before going any further is always a good
> thing.
In that case I'm glad to help, and glad to put in a little more of my
precious time than usual. :)
> I've found some more issues while using more and more the port and the
> scripts, for example, the openerp-server port should create a new
> account _openerp (to use my rc.d script properly) and the openerp-web
> port should check if the user exist and, if not, create it (useful for
> servers that will serve the web client, connecting to a remote openerp
> server).
Actually the port itself should create the user, we generally don't have
rc.d scripts do that. If the sysadmin wants a different user than the
default it's up to them to configure that before starting the service.
> I'm working on improved versions of the script, will send them soon to
> the list.
>
> Thnk you a lot Doug
>
> (And thnx for the portmaster tool too! ;D)
heh, you're welcome.
Doug
--
Improve the effectiveness of your Internet presence with
a domain name makeover! http://SupersetSolutions.com/
Computers are useless. They can only give you answers.
-- Pablo Picasso
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?4B5F398E.5060500>