Merge lp://qastaging/~ted/snapcraft/wrapper-path into lp://qastaging/~snappy-dev/snapcraft/core

some pita comments.

Leo's comments are fair. I've added a couple of my own too.

Calling out to a shell is a bit gross, but I get it. I imagine we could parse the PATH options ourselves. But maybe that's grosser code wise.

On Thu, 2015-07-23 at 19:00 +0000, Leo Arias wrote:

> > === modified file 'snapcraft/cmds.py'
> > --- snapcraft/cmds.py 2015-07-22 21:05:20 +0000
> > +++ snapcraft/cmds.py 2015-07-23 18:33:26 +0000
> > @@ -65,7 +65,18 @@
> > except Exception:
> > pass
> >
> > - script = "#!/bin/sh\n%s\nexec %s $*" % (snapcraft.common.assemble_env().replace(snapcraft.common.snapdir, "$SNAP_APP_PATH"), '"$SNAP_APP_PATH/%s"' % relexepath)
> > + wrapexec = '"$SNAP_APP_PATH/%s"' % relexepath
>
> please use .format instead of %
>
> > + if not os.path.exists(exepath) and '/' not in relexepath:
> > + # If it doesn't exist it might be in the path
> > + with tempfile.NamedTemporaryFile() as tempf:
> > + script = "#!/bin/sh\n%s\nwhich %s" % (snapcraft.common.assemble_env(), relexepath)
>
> I find it clearer when a newline is used after the \n, like:
>
> script = ('#!/bin/sh\n' +
> '{}\n'.format(snapcraft.common.assemble_env()) +
> 'which {}'.format(relexepath))

Fixed r111.

> > + tempf.write(bytes(script, 'UTF-8'))
> > + if snapcraft.common.run(['/bin/sh', tempf.name], cwd=snapcraft.common.snapdir):
> > + wrapexec = relexepath
> > + else:
> > + snapcraft.common.log("Warning: unable to find %s in the path" % (relexepath))
>
> use logger.info('Warning: unable to find {} in the path', relexepath), like in https://code.launchpad.net/~elopio/snapcraft/fix1476452-python_log/+merge/265354

Fixed r112.

On Fri, 2015-07-24 at 16:40 +0000, Michael Terry wrote:

> Calling out to a shell is a bit gross, but I get it. I imagine we
> could parse the PATH options ourselves. But maybe that's grosser code
> wise.

Yes, wanted to be immune to what ever shell-isms people do. No reason to
write a complete parser.

> > + with tempfile.NamedTemporaryFile() as tempf:
> > + script = "#!/bin/sh\n%s\nwhich %s" % (snapcraft.common.assemble_env(), relexepath)
> > + tempf.write(bytes(script, 'UTF-8'))
>
>
> I think you can open the file as a string file (mode='w+'), instead of
> a bytes file, to avoid that bit of grossness.

Fixed r114.

> > + if snapcraft.common.run(['/bin/sh', tempf.name], cwd=snapcraft.common.snapdir):
>
>
> Don't you need to flush the file to ensure it's written to disk before
> this?

Fixed r115.

> > + wrapexec = relexepath
> > + else:
> > + snapcraft.common.log("Warning: unable to find %s in the path" % (relexepath))
>
>
> That should probably be a fatal error too, eh?

Well, I didn't want to change previous behavior there. We weren't
failing before, just because we know more, I'm not sure we should fail
now.

Nice, thanks Ted!