Merge lp://staging/~kentb/ubuntu/quantal/ubiquity/add-custom-dm-scripts into lp://staging/ubiquity

 review needs-fixing

Thanks for your patch. I'm fine with the general notion, but have a few
comments which I think need to be tidied up.

On Tue, Jun 26, 2012 at 09:24:25PM -0000, Kent Baxley wrote:
> === modified file 'bin/ubiquity-dm'
> --- bin/ubiquity-dm 2012-06-20 08:53:56 +0000
> +++ bin/ubiquity-dm 2012-06-26 21:23:22 +0000
> @@ -261,6 +261,20 @@
> os.environ['GVFS_DISABLE_FUSE' ] = '1'
>
> ck_open_session(self.uid)
> +
> + #run simple, custom scripts during install time

Space after #.

> + if sys.argv[4] == '/usr/bin/ubiquity':

I know you're copying an existing bug (feel free to fix it), but this
should really be program[0] rather than sys.argv[4], and it should use
os.path.basename so that it won't break if we stop passing a full path
to the program.

Rather than doing this multiple times, perhaps you could extract the
basename of the program being called at the top of DM.run, and then
these two new checks plus the one existing check could use it?

> + for root, dirs, files in os.walk("/usr/lib/ubiquity/dm-scripts/install"):
> + for file in files:
> + install_script = os.path.join(root, file)
> + subprocess.Popen(install_script, stdin=null, stdout=logfile, stderr=logfile, preexec_fn=self.drop_privileges)

Please wrap to no more than 79 characters. (Yes, not all the existing
code follows this, but that's a bug.)

> + for root, dirs, files in os.walk("/usr/lib/ubiquity/dm-scripts/oem"):
> + for file in files:
> + oem_script = os.path.join(root, file)
> + subprocess.Popen(oem_script, stdin=null, stdout=logfile, stderr=logfile, preexec_fn=self.drop_privileges)

Please use four-space indents consistently.

The repetition here suggests that a helper method might be in order,
something like:

    def run_hooks(self, hookdir):
        if os.path.isdir(hookdir):
            for root, dirs, files in os.walk(hookdir):
                for file in files:
                    script = os.path.join(root, file)
                    subprocess.Popen(
                        script, stdin=null, stdout=logfile, stderr=logfile,
                        preexec_fn=self.drop_privileges)

I'm uncomfortable with the way this uses subprocess.Popen rather than
subprocess.call. I know that in your specific use case you want to fire
something off in the background, but maybe the next person who uses this
facility will want to do something synchronously instead. If you use
subprocess.call, then the hook script can always use things like "&" in
shell to background commands it runs. If you use subprocess.Popen, the
hook script has no way to move commands into the foreground.

Finally, I think it's overkill to do a recursive directory walk here. I
would just do something like this, much like the code for running
target-config hooks:

        if os.path.isdir(hookdir):
            # Exclude hooks containing '.', so that *.dpkg-* et al are avoided.
            hooks = [entry for entry in os.listdir(hookdir)
                     if '.' not in entry]
            for hookentry in hooks:
           ...

> review needs-fixing
>
> Thanks for your patch. I'm fine with the general notion, but have a few
> comments which I think need to be tidied up.
>
>

Hi Colin,

Thanks for all the helpful pointers. I've made some changes to ubiquity-dm and I've gotten rid of the default directory entries per your request. Please let me know if my changes are closer to what you had in mind.

The only thing I wasn't sure about was the 'existing check' that you were referring to in ubiquity-dm. I'm guessing it was this one?:

http://bazaar.launchpad.net/~ubuntu-installer/ubiquity/trunk/revision/5511

If so, I can make that tweak if you want so that the check does something like:

program_basename == 'oem-config-wrapper'

...but, I'm not sure of the actual nature of the bug itself that that particular tweak addressed.

Thanks, again, for your help. I'll be glad to make any other needed changes.

Hi Colin,

Any additional comments on the code cleanup? Thanks.

On Fri, Jun 29, 2012 at 03:01:24PM -0000, Kent Baxley wrote:
> Thanks for all the helpful pointers. I've made some changes to
> ubiquity-dm and I've gotten rid of the default directory entries per
> your request. Please let me know if my changes are closer to what you
> had in mind.

Yes, thanks. Though I wouldn't bother with:

    hookdir = '/usr/lib/ubiquity/dm-scripts/install'
    self.run_hooks(hookdir)

... just say "self.run_hooks('/usr/lib/ubiquity/dm-scripts/install')"
instead.

Also, I do think you should add "if '.' not in entry" to your list
comprehension that generates the list of hooks, otherwise it's a bit of
a landmine for the future due to the way dpkg unpacking works. It
matters less for non-conffiles, admittedly, but I'd still be more
comfortable with it that way from the start rather than finding that we
need to retrofit this later.

> The only thing I wasn't sure about was the 'existing check' that you were referring to in ubiquity-dm. I'm guessing it was this one?:
>
> http://bazaar.launchpad.net/~ubuntu-installer/ubiquity/trunk/revision/5511
>
> If so, I can make that tweak if you want so that the check does something like:
>
> program_basename == 'oem-config-wrapper'
>
> ...but, I'm not sure of the actual nature of the bug itself that that particular tweak addressed.

Yes, that's right, and please do. The exact bug in question doesn't
really matter here as this is refactoring.

This will be fine to merge after these minor adjustments - just poke me
on IRC or something.

 review approve

Thanks, Colin. I've made the changes and I'll ping you on IRC as well.