Merge lp:~maxb/udd/environment-setup into lp:udd

[Max Bowsher]

[Max Bowsher (maxb)]

Recording some (inconclusive) IRC discussion:

11:57 < mgz> maxb: seems pretty reasonable to me, might just want vila to look it over too
12:06 < vila> maxb: the scripts have been rewritten by jml/james_w because they want to reuse udd in a different context so they wanted some dependencies *out* of the scripts (adding pkgimport.conf starts addressing that), putting
              everything *in* again goes in the opposite direction, you should see with them to reach a consensus
12:08 < vila> maxb: have a look at udd/iconfig.py too which already defines pi.install_dir and pi.base_dir based on _root_dir that you're duplicating with udd_scripts_root
12:09 * vila bbl
12:26 < maxb> vila: jml / james_w are welcome to re-use the udd.* modules, but making our bin/ scripts do the right thing when invoked without wrapper shell scripts shouldn't affect them, I would think
12:27 < jml> maxb: I haven't looked at this change in particular (kind of caught up in something else atm)....
12:27 < jml> maxb: but personally, I think the best thing we can do for this code-base is to split the "watch packages on Launchpad" bit away from the "import packages into branches" bit.
12:29 < maxb> Sure, but right now I just want scripts that don't die if I don't have a PYTHONPATH set :-)

[lp:~maxb/udd/environment-setup]

574. By Max Bowsher on 2012-04-11

As jml / james_w use the udd codebase for some other stuff, move unrelated
environment settings out of the common imported file.

Happily, they're only relevant to import-package anyway, and that's purely
UDD-specific, so put them there instead.

[James Westby (james-w)]

10 +# bzr-builddeb requires distro-info
11 +sys.path.insert(0, os.path.join(udd_root, 'distro-info', 'python'))

I think that might only be needed by import-package as well?

I think I'm ok with this landing, as I don't think it would have an adverse
affect on our deployment. However, I don't think that it is the way to go.
I think that the deployment of the scripts should be separated from the code,
so that it can be re-used (as we are, and others may want to.)

Elsewhere we've started adding "py.sh" scripts to the deployment (via puppet
in those cases) that set up the environment for running the scripts. It's
one more thing to put in the command-line, but it allows for any customization
without cluttering the codebase. Maybe we could use that model here.

Thanks,

James

[Max Bowsher (maxb)]

You're right, I'll move the distro-info bit.

I *do* think this is the way to go, as I feel it's an outright bug that Python scripts require external assistance to import modules which are guaranteed to be at a given relative path from the scripts themselves.

Maybe, in some super-complex deployment scenarios, a "py.sh" is the way to go. But UDD isn't a super-complex deployment scenario, it's easily in reach for the scripts to just do the right thing, without wrappers.

Amongst other things, I want to be able to just tab-complete something in bin/ and have it work! :-)

[lp:~maxb/udd/environment-setup]

575. By Max Bowsher on 2012-04-16

Move the distro-info special case to import-package too.
Fix a couple of missing imports found via pyflakes.

[Vincent Ladeuil (vila)]

> You're right, I'll move the distro-info bit.
>
> I *do* think this is the way to go, as I feel it's an outright bug that Python
> scripts require external assistance to import modules which are guaranteed to
> be at a given relative path from the scripts themselves.

Actually, there are not guaranteed to be there, that's specific to the jubany deployment and even there, in the long term, we should rely on packaged versions...

>
> Maybe, in some super-complex deployment scenarios, a "py.sh" is the way to go.
> But UDD isn't a super-complex deployment scenario, it's easily in reach for
> the scripts to just do the right thing, without wrappers.
>
> Amongst other things, I want to be able to just tab-complete something in bin/
> and have it work! :-)

Which I achieve by sourcing fixit.sh ;)

[Max Bowsher (maxb)]

> > You're right, I'll move the distro-info bit.
> >
> > I *do* think this is the way to go, as I feel it's an outright bug that
> Python
> > scripts require external assistance to import modules which are guaranteed
> to
> > be at a given relative path from the scripts themselves.
>
>
> Actually, there are not guaranteed to be there, that's specific to the jubany
> deployment and even there, in the long term, we should rely on packaged
> versions...

I'm referring to the udd.* modules, which are in the same bzr branch as the bin/ directory - that's a pretty hard guarantee where they are - unless you're saying people are deploying udd code not via a bzr branch?

> > Maybe, in some super-complex deployment scenarios, a "py.sh" is the way to
> go.
> > But UDD isn't a super-complex deployment scenario, it's easily in reach for
> > the scripts to just do the right thing, without wrappers.
> >
> > Amongst other things, I want to be able to just tab-complete something in
> bin/
> > and have it work! :-)
>
> Which I achieve by sourcing fixit.sh ;)

But that's a hack, and we shouldn't have to do it.

[Vincent Ladeuil (vila)]

> > > You're right, I'll move the distro-info bit.
> > >
> > > I *do* think this is the way to go, as I feel it's an outright bug that
> > Python
> > > scripts require external assistance to import modules which are guaranteed
> > to
> > > be at a given relative path from the scripts themselves.
> >
> >
> > Actually, there are not guaranteed to be there, that's specific to the
> jubany
> > deployment and even there, in the long term, we should rely on packaged
> > versions...
>
> I'm referring to the udd.* modules, which are in the same bzr branch as the
> bin/ directory - that's a pretty hard guarantee where they are - unless you're
> saying people are deploying udd code not via a bzr branch?

No, you're also referring (well, your change does), to bzrlib and the
plugins which are specific to the jubany deployment.

>
>
> > > Maybe, in some super-complex deployment scenarios, a "py.sh" is the way to
> > go.
> > > But UDD isn't a super-complex deployment scenario, it's easily in reach
> for
> > > the scripts to just do the right thing, without wrappers.
> > >
> > > Amongst other things, I want to be able to just tab-complete something in
> > bin/
> > > and have it work! :-)
> >
> > Which I achieve by sourcing fixit.sh ;)
>
> But that's a hack, and we shouldn't have to do it.

Well, yeah it's hack, but so is your change, setting env variables *inside*
the scripts is a hack, there are always risks that they will be used by the
interpreter earlier than you think or, more probably, earlier that someone
not knowing the the implementation of bzrlib for BZR_PLUGIN_PATH, BZR_EMAIL
or the python interpreter implementation for LANG.

Also:

153 -
154 -
155 -if __name__ == '__main__':
156 - main()
157 -
158 +main()

removes the ability to ever test the scripts themselves by loading them
without triggering main.

By the way, did you run the test suite for your change ?

What I'm saying here is that this change don't make it easier to reuse udd
outside of jubany and as such doesn't go in the right direction.

The actual code triplicate the definitions of the env variables but do so in
a way that at least outline the different contexts where they are needed:

1 - crontab (no way that I know of to source anything there nor execute any
    external code that can set these variables),

2 - etc-init.d-mass-import which is run by root. It's easier to have changes
    in the same file so losas can review them when updating the version that
    is really run (unless you can convince them to use a symlink to our
    branch which I highly doubt they will agree with),

3 - manual access to jubany

While you bring a small enhancement to 3, 1 and 2 suffer, so overall I don't
think it's a progress.

I'd rather remove the need for the multiple definitions by relying on
packaged dependencies.

[Max Bowsher (maxb)]

> No, you're also referring (well, your change does), to bzrlib and the
> plugins which are specific to the jubany deployment.

OK, you're right there, those are sort-of included too. But uppermost in my mind was the way that the scripts can't even find the path to their own udd.* library code without outside assistance, which feels like the biggest illustrator of the bug to me.

> Well, yeah it's hack, but so is your change, setting env variables *inside*
> the scripts is a hack, there are always risks that they will be used by the
> interpreter earlier than you think or, more probably, earlier that someone
> not knowing the the implementation of bzrlib for BZR_PLUGIN_PATH, BZR_EMAIL
> or the python interpreter implementation for LANG.

I really disagree that setting environment variables inside the scripts is a hack. It's not really any different from initialization functions that need to be called in certain ordering relationships.

A case to illustrate why I think this would be a REALLY good idea: the importer is currently slogging through a queue of 22k failures brought about by the importer somehow having been run without BZR_PLUGIN_PATH set. It's this sort of thing which makes me think that critical parts of the system should be integrated into the Python itself., rather than left in various wrappers.

> Also:
>
> 153 -
> 154 -
> 155 -if __name__ == '__main__':
> 156 - main()
> 157 -
> 158 +main()
>
> removes the ability to ever test the scripts themselves by loading them
> without triggering main.

No! Whilst I'm fully aware and approve of this particular Python idiom where it makes sense, it is just meaningless extra characters in each of the bin/* executables. Note that those executables do almost nothing except import and call a module where all of the real code lives, precisely to make it testable. Note also that those executables don't end with .py, so they are not importable at all (unless you play tricks with __import__).

> By the way, did you run the test suite for your change ?

Not initially, sorry, but it appears to be since the my last revision 6 days ago.

> What I'm saying here is that this change don't make it easier to reuse udd
> outside of jubany and as such doesn't go in the right direction.

Actually, it does, I think. It removes need for (use-specific!) wrapper scripts in many cases, and it moves bits which are jubany-specific into the bin/import-package script, which is already specific to the jubany use-case.

> The actual code triplicate the definitions of the env variables but do so in
> a way that at least outline the different contexts where they are needed:
>
> 1 - crontab (no way that I know of to source anything there nor execute any
> external code that can set these variables),

Sure there is, just run a command of the style (. my-env-file; ./my-command.py); but even better if the command itself can do the right thing.

> 2 - etc-init.d-mass-import which is run by root. It's easier to have changes
> in the same file so losas can review them when updating the version that
> is really run (unless you can convince them to use a symlink to our
> branch which I hig...

[James Westby (james-w)]

On Sun, 22 Apr 2012 11:41:20 -0000, Max Bowsher <email address hidden> wrote:
> > No, you're also referring (well, your change does), to bzrlib and the
> > plugins which are specific to the jubany deployment.
>
> OK, you're right there, those are sort-of included too. But uppermost
> in my mind was the way that the scripts can't even find the path to
> their own udd.* library code without outside assistance, which feels
> like the biggest illustrator of the bug to me.

Agreed on that point. I think that fixing that problem would be good,
it's the other parts that I find contentious. My apologies for not
noticing this distinction earlier.

> I think, given the level of unexpected controversy that has ensued from this branch, I might split it up into several:
>
> 1) bin/_path.py *only* dealing with adding the udd.* modules to the path

+1 to that.

> 2) bzrlib and distro-info installed via a "pylib" directory, replacing current methods

I'm +0 on that.

> 3) Remove redundant if __name__ == '__main__'

+0 too.

> 4) BZR_PLUGIN_PATH, BZR_EMAIL, LANG

I'm still not sure about this.

You say that import-package is jubany-specific, which I somewhat agree
with (it's certainly specific to the package importer, but if we want a
staging instance then there will be two machines involved for just
that.) Splitting it from the rest of the udd tree would help re-inforce
that.

Given that nothing else uses it I wouldn't block changing this, and
setting BZR_EMAIL and LANG are fine in any case I think¸ I just think
there are better ways of handling BZR_PLUGIN_PATH and PYTHONPATH for
external projects.

Thanks,

James

[Max Bowsher (maxb)]

This MP is now withdrawn, partially replaced by: https://code.launchpad.net/~maxb/udd/pythonpath-1/+merge/104696

with further MPs to replace other parts of this to come.

[lp:~maxb/udd/environment-setup]

576. By Max Bowsher on 2012-05-04

Merge trunk.

577. By Max Bowsher on 2012-08-24

Merge

Unmerged revisions

577. By Max Bowsher on 2012-08-24

Merge

576. By Max Bowsher on 2012-05-04

Merge trunk.

575. By Max Bowsher on 2012-04-16

Move the distro-info special case to import-package too.
Fix a couple of missing imports found via pyflakes.

574. By Max Bowsher on 2012-04-11

As jml / james_w use the udd codebase for some other stuff, move unrelated
environment settings out of the common imported file.

Happily, they're only relevant to import-package anyway, and that's purely
UDD-specific, so put them there instead.

573. By Max Bowsher on 2012-04-09

Factor out duplicated environent setup functionality from the /etc/init.d
script, the crontab, and fixit.sh into code imported by all the scripts.

This results in the scripts being directly runnable without prior effort to
configure appropriate environment variables being needed.

Also stop exporting BASEDIR, since it's not needed elsewhere, and stop
exporting PATH, which by definition must be already exported to be useful.

=== modified file 'bin/_path.py'
--- bin/_path.py	2012-05-04 07:42:36 +0000
+++ bin/_path.py	2012-08-24 07:58:20 +0000
@@ -2,6 +2,9 @@
 import sys
 
 udd_scripts_root = os.path.dirname(os.path.dirname(os.path.abspath(__file__)))
+udd_root = os.path.dirname(udd_scripts_root)
 
+# If a local bzr is installed, use it
+sys.path.insert(0, os.path.join(udd_root, 'bzr'))
 # scripts branch root contains the udd library
 sys.path.insert(0, udd_scripts_root)

=== modified file 'bin/import-package'
--- bin/import-package	2012-05-23 15:10:06 +0000
+++ bin/import-package	2012-08-24 07:58:20 +0000
@@ -1,5 +1,19 @@
 #!/usr/bin/python
 import sys
 import _path
+import sys
+import os
+
+# Typically contains desired version of bzr-builddeb
+os.environ['BZR_PLUGIN_PATH'] = os.path.join(_path.udd_scripts_root, 'plugins')
+
+# bzr-builddeb requires distro-info
+sys.path.insert(0, os.path.join(_path.udd_root, 'distro-info', 'python'))
+
+os.environ['BZR_EMAIL'] = 'Package Import Robot <[e-mail]>'
+
+# Forcibly adopt a UTF-8 locale
+os.environ['LANG'] = 'en_GB.UTF-8'
+
 from udd.scripts.import_package import main
 sys.exit(main(sys.argv))

=== modified file 'etc-init.d-mass-import'
--- etc-init.d-mass-import	2012-08-09 15:10:57 +0000
+++ etc-init.d-mass-import	2012-08-24 07:58:20 +0000
@@ -1,21 +1,15 @@
 #!/bin/sh
 
-export BASEDIR=/srv/package-import.canonical.com/new
+BASEDIR=/srv/package-import.canonical.com/new
 # We need ${BASEDIR}/scripts for our local dpkg-mergechangelogs (see
 # http://pad.lv/1016471)
-export PATH=${BASEDIR}/scripts:/usr/local/sbin:/usr/local/bin:/sbin:/bin:/usr/sbin:/usr/bin
-# * If a local bzr is installed, use it
-# * bzr-builddeb requires distro-info
-export PYTHONPATH=${BASEDIR}/bzr:${BASEDIR}/distro-info/python
+PATH=${BASEDIR}/scripts:/usr/local/sbin:/usr/local/bin:/sbin:/bin:/usr/sbin:/usr/bin
 NAME=mass-import
 DAEMON=${BASEDIR}/scripts/bin/mass-import
 PIDFILE=${BASEDIR}/mass-import.pid
 STOPFILE=${BASEDIR}/STOP_PLEASE
 USER=pkg_import
-export LANG="C.UTF-8"
-export BZR_PLUGIN_PATH=${BASEDIR}/scripts/plugins/
 export TMPDIR=${BASEDIR}/tmp
-export BZR_EMAIL="Package Import Robot <[e-mail]>"
 
 usage() {
     echo "Usage: $0 {start|stop|restart|status|graceful-stop}"

=== modified file 'fixit.sh'
--- fixit.sh	2012-05-04 07:42:36 +0000
+++ fixit.sh	2012-08-24 07:58:20 +0000
@@ -11,15 +11,7 @@
 # plan is to get rid of the useless ones and integrate the useful
 # ones in a better place.
 
-export BASEDIR=/srv/package-import.canonical.com/new
-export PATH=${BASEDIR}/scripts/bin:${PATH}
-# * If a local bzr is installed, use it
-# * bzr-builddeb requires distro-info
-export PYTHONPATH=${BASEDIR}/bzr:${BASEDIR}/distro-info/python
-export BZR_PLUGIN_PATH=${BASEDIR}/scripts/plugins
-export LANG="en_GB.UTF-8"
-export BZR_EMAIL="Package Import Robot <[e-mail]>"
-
+PATH=/srv/package-import.canonical.com/new/scripts/bin:${PATH}
 
 lp_url() {
     local package dist series

=== modified file 'importer.crontab'
--- importer.crontab	2012-08-09 16:10:58 +0000
+++ importer.crontab	2012-08-24 07:58:20 +0000
@@ -1,8 +1,5 @@
 BASE_DIR=/srv/package-import.canonical.com/new/scripts
 SCRIPTS_DIR=/srv/package-import.canonical.com/new/scripts/bin
-# * If a local bzr is installed, use it
-# * bzr-builddeb requires distro-info
-PYTHONPATH=/srv/package-import.canonical.com/new/bzr:/srv/package-import.canonical.com/new/distro-info/python
 # m h  dom mon dow   command
 CHROOT="schroot -c quantal --"
 PYTHON=/usr/bin/python