Code review comment for lp:~maxb/udd/environment-setup

> 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 highly doubt they will agree with),

*But*, its even better to move the settings which only affect code that runs as the pkg_import user out of that root-managed file entirely, so the losas don't need to review them!

> 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.

How have 1 and 2 suffered?

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

I'd rather use packaged dependencies where appropriate, but acknowledge that we will always have times when we, as a single application, want to move faster than it is appropriate for the jubany system install to move.

To that end, I'm even contemplating proposing that we shift to creating a single "pylib" directory, and place the distro-info and bzrlib that we use there, rather than having separate PYTHONPATH additions for each project we want to add. But that's a discussion for a different MP or the mailing list.

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

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

3) Remove redundant if __name__ == '__main__'

4) BZR_PLUGIN_PATH, BZR_EMAIL, LANG