Merge lp://qastaging/~mterry/snapcraft/run-as-list into lp://qastaging/~snappy-dev/snapcraft/core

Proposed by Michael Terry
Status: Merged
Approved by: Michael Terry
Approved revision: 78
Merged at revision: 78
Proposed branch: lp://qastaging/~mterry/snapcraft/run-as-list
Merge into: lp://qastaging/~snappy-dev/snapcraft/core
Diff against target: 168 lines (+26/-37)
7 files modified
snapcraft/__init__.py (+1/-4)
snapcraft/cmds.py (+4/-4)
snapcraft/common.py (+3/-4)
snapcraft/plugins/autotools_project.py (+8/-12)
snapcraft/plugins/cmake_project.py (+7/-11)
snapcraft/plugins/make_project.py (+2/-1)
tests/plainbox/data/local-plugin/parts/plugins/local_plugin.py (+1/-1)
To merge this branch: bzr merge lp://qastaging/~mterry/snapcraft/run-as-list
Reviewer Review Type Date Requested Status
Michael Vogt (community) Approve
Review via email: mp+264650@code.qastaging.launchpad.net

Commit message

Make all commands given to our run helper be lists, rather than allowing strings.

Description of the change

Make all commands given to our run helper be lists, rather than allowing strings.

To post a comment you must log in.
78. By Michael Terry

Move comment closer to code it is commenting on

Revision history for this message
Michael Vogt (mvo) wrote :

Looks great! One small thing I noticed inline but its unrelated to the branch so feel free to ignore.

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

About your comment on the "if True:"...

It originally was an "if False:" that I would turn into "if True" when debugging, yes. But it has felt like good feedback when running snapcraft to see the commands. So I kind of like having it in, even for production.

Do you think it's useful or the end user or should it only be shown in a --verbose situation?

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches