Code review comment for lp:~stub/launchpad/trivial

On Fri, Jul 24, 2009 at 3:00 AM, Christian Reis<email address hidden> wrote:
> On Thu, Jul 23, 2009 at 08:38:44AM -0000, Stuart Bishop wrote:
>> A startup script for the Librarian.
>> bin/run isn't suitable for use on production as it launches an appserver.
>
> Could it not be made to run the librarian as well? I guess at this point
> whatever's easiest to do and causes less problems is sanest.

It was easier to add the new script. The existing code runs daemons in
foreground mode but puts them in the background using
subprocess.Popen - it is designed for devs and around having the
services shutdown when the Launchpad instance does. Its also
undocumented with no command line help, and that doesn't look fixable
without maybe breaking other call sites that depend on it such as
ec2test.

>> === modified file 'Makefile'
>>  stop_codebrowse:
>>       $(PY) sourcecode/launchpad-loggerhead/stop-loggerhead.py
>>
>> +start_librarian: build
>> +     bin/start_librarian
>> +
>> +stop_librarian:
>> +     bin/killservice librarian
>
> Are these created by buildout automatically? I haven't seen any of this
> code before, sorry.

Yes.

In buildout.cfg, there are stanzas that tell it to generate these
scripts - they are wrappers with the correct !# set, some boilerplate
code to set the pythopath correctly, and then invokes the entry point
method (canonical.launchpad.scripts.runlaunchpad.start_librarian).

>> === modified file 'lib/canonical/launchpad/scripts/runlaunchpad.py'
>
> What else uses this script? /bin/run?

Yes.

>> @@ -84,7 +85,6 @@
>>          tacfile = make_abspath(self.tac_filename)
>>
>>          args = [
>> -            sys.executable,
>>              TWISTD_SCRIPT,
>
> Was this broken or just unnecessary? I can't see what this code does
> from the context.

It was unnecessary - it needlessly tied us to using the same version
of Python that was used to invoke this script. Buildout handles this
for us, adding the correct interpreter and python paths per buildout
configuration.

>> -        # We want to make sure that the Launchpad process will have the
>> -        # benefit # of all of the dependency paths inserted by the buildout
>> -        # bin/run script. We pass them via PYTHONPATH.
>> -        env = dict(os.environ)
>> -        env['PYTHONPATH'] = os.path.pathsep.join(sys.path)
>
> Will this not cause problems anywhere else?

No - buildout handles this for us now by building _pythonpath.py will
all the libraries we are supposed to need. If you don't like it, we
can land this bit another time - it is harmless and just a drive by
cleanup to keep our variables down when trying to debug things.

--
Stuart Bishop <email address hidden>
http://www.stuartbishop.net/