Code review comment for lp://qastaging/~mvo/snapcraft/run-run-as-list

Revision history for this message
Michael Terry (mterry) wrote :

I'm OK with the direction.

Right now we have a bug: run() doesn't handle arguments with spaces (or other such shell-meaningful characters) well. So this fixes that and that's good.

We don't need the tempfile. But here's my only reservation: I kind of liked the idea that plugins could have interesting bits of shell in their environment setup. The examples we have now just append to lists. But maybe a plugin might want to chop up a variable with some sort of "$(echo $VAR | sed GROSSSTUFF)" or do an optional declaration (like 'if [ "$SNAPPY_ARCH" = "amd64" ]; then....' (which admittedly, they couldn't easily do now unless they insert a newline into their env() array.

The arch stuff might be a bad example, because snapcraft always builds on the same arch as the runtime. And I can't actually think of another sensible reason to want a conditional env var.

But still, I could imagine a plugin doing the $() thing. Do you think it's valuable to allow that, or just asking for trouble?

« Back to merge proposal