Merge lp://qastaging/~mvo/snapcraft/run-run-as-list into lp://qastaging/~snappy-dev/snapcraft/core
Proposed by
Michael Vogt
| Status: | Work in progress |
|---|---|
| Proposed branch: | lp://qastaging/~mvo/snapcraft/run-run-as-list |
| Merge into: | lp://qastaging/~snappy-dev/snapcraft/core |
| Prerequisite: | lp://qastaging/~mterry/snapcraft/run-as-list |
| Diff against target: |
40 lines (+10/-9) 1 file modified
snapcraft/common.py (+10/-9) |
| To merge this branch: | bzr merge lp://qastaging/~mvo/snapcraft/run-run-as-list |
| Related bugs: |
| Reviewer | Review Type | Date Requested | Status |
|---|---|---|---|
| Snappy Developers | Pending | ||
|
Review via email:
|
|||
Description of the change
Another RFC branch - this one avoids writing a tempfile in snapcraft.
The way it builds the environment is gross right now I'm happy to clean that up *if* the direction looks ok. If there are other reasons to keep the tempfile that I'm not aware of I won't bother :) The cleanup would probably be to make the plugins use a friendlier function/data structure, something like append_env(key, value) and internally it would ensure that e.g. PATH is appended/prepended. But I have not put much thought into that yet, ideas/suggestsion very welcome.
To post a comment you must log in.
Revision history for this message
| Michael Terry (mterry) wrote | # |
Revision history for this message
| Michael Vogt (mvo) wrote | # |
Revision history for this message
| Sergio Schvezov (sergiusens) wrote | # |
Unmerged revisions
- 79. By Michael Vogt
-
make run() uses subprocess.call() instead of writing a temp file (but in a terrible hacky way right now)
Preview Diff
[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
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?