Merge lp:~lool/linaro-image-tools/optional-sudo into lp:linaro-image-tools/11.11
- optional-sudo
- Merge into trunk
Status: | Merged |
---|---|
Merged at revision: | 279 |
Proposed branch: | lp:~lool/linaro-image-tools/optional-sudo |
Merge into: | lp:linaro-image-tools/11.11 |
Diff against target: |
160 lines (+37/-21) 4 files modified
linaro-hwpack-install (+11/-6) linaro-media-create (+15/-11) linaro_media_create/cmd_runner.py (+3/-3) linaro_media_create/tests/test_media_create.py (+8/-1) |
To merge this branch: | bzr merge lp:~lool/linaro-image-tools/optional-sudo |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
James Westby (community) | Approve | ||
Review via email: mp+47907@code.launchpad.net |
Commit message
Description of the change
- 276. By Loïc Minier
-
Mock getuid() when testing cmd_runner and test both the sudo needed and sudo
not needed cases.
Loïc Minier (lool) wrote : | # |
I update the cmd_runner tests in r276 now; I realize that we ought to change the whole testsuite to assume "regular user" (getuid() != 0), but changing the Popen mock class to mock os.getuid() required changing the MockSomething fixture in non-trivial ways, so I'd like to consult with salgado on doing this nicely.
We could either change cmd_runner to take a force_sudo/
Guilherme Salgado (salgado) wrote : | # |
It's not clear to me why we ought to change the whole testsuite to assume "regular user". Is it because when building a package the tests will be executed as root and that would now cause the ones that use cmd_runner.Popen() to fail?
Loïc Minier (lool) wrote : | # |
When building a package, the tests will be run as a regular user; perhaps under fakeroot, I didn't check.
But I think the uid of the calling user shouldn't matter, we should build the testsuite to work whatever the uid. I'm proposing we change the testsuite to setup things so that it looks like a regular user is running it so that we get the chance to include sudo in the expect commands; that way, we include the "this piece needs to be run as root" information in all the Popen tests.
So I'm suggesting that whatever the calling user, we should divert getuid() to return non-zero, and we should expect commands to look like:
[sudo command-
command-
etc.
Loïc Minier (lool) wrote : | # |
I just found a stronger argument for avoiding sudo: this allows strace-ing the whole of linaro-
Preview Diff
1 | === modified file 'linaro-hwpack-install' | |||
2 | --- linaro-hwpack-install 2011-01-28 19:50:48 +0000 | |||
3 | +++ linaro-hwpack-install 2011-01-29 19:21:18 +0000 | |||
4 | @@ -38,6 +38,11 @@ | |||
5 | 38 | APT_GET_OPTIONS="Dir::Etc::SourceList=${SOURCES_LIST_FILE}" | 38 | APT_GET_OPTIONS="Dir::Etc::SourceList=${SOURCES_LIST_FILE}" |
6 | 39 | SUPPORTED_FORMATS="1.0" # A space-separated list of hwpack formats. | 39 | SUPPORTED_FORMATS="1.0" # A space-separated list of hwpack formats. |
7 | 40 | 40 | ||
8 | 41 | sudo="sudo" | ||
9 | 42 | if [ $(id -u) -eq 0 ]; then | ||
10 | 43 | sudo="" | ||
11 | 44 | fi | ||
12 | 45 | |||
13 | 41 | die() { | 46 | die() { |
14 | 42 | echo -e "$@" | 47 | echo -e "$@" |
15 | 43 | exit 1 | 48 | exit 1 |
16 | @@ -79,7 +84,7 @@ | |||
17 | 79 | # Ensure our temp dir and apt sources are removed. | 84 | # Ensure our temp dir and apt sources are removed. |
18 | 80 | echo -n "Cleaning up ..." | 85 | echo -n "Cleaning up ..." |
19 | 81 | rm -rf $TEMP_DIR | 86 | rm -rf $TEMP_DIR |
21 | 82 | sudo apt-get update -qq | 87 | $sudo apt-get update -qq |
22 | 83 | echo "Done" | 88 | echo "Done" |
23 | 84 | } | 89 | } |
24 | 85 | 90 | ||
25 | @@ -128,14 +133,14 @@ | |||
26 | 128 | done < $stripped_file | 133 | done < $stripped_file |
27 | 129 | 134 | ||
28 | 130 | if [ $should_install -eq 1 ]; then | 135 | if [ $should_install -eq 1 ]; then |
30 | 131 | sudo cp $file /etc/apt/sources.list.d/hwpack.$filename | 136 | $sudo cp $file /etc/apt/sources.list.d/hwpack.$filename |
31 | 132 | fi | 137 | fi |
32 | 133 | done | 138 | done |
33 | 134 | 139 | ||
34 | 135 | # Import the OpenPGP keys for the files installed above. | 140 | # Import the OpenPGP keys for the files installed above. |
35 | 136 | for filename in $(ls "${HWPACK_DIR}"/sources.list.d.gpg/); do | 141 | for filename in $(ls "${HWPACK_DIR}"/sources.list.d.gpg/); do |
36 | 137 | file="${HWPACK_DIR}"/sources.list.d.gpg/$filename | 142 | file="${HWPACK_DIR}"/sources.list.d.gpg/$filename |
38 | 138 | sudo apt-key add $file | 143 | $sudo apt-key add $file |
39 | 139 | done | 144 | done |
40 | 140 | 145 | ||
41 | 141 | # Add one extra apt source for the packages included in the hwpack and make | 146 | # Add one extra apt source for the packages included in the hwpack and make |
42 | @@ -145,7 +150,7 @@ | |||
43 | 145 | cat /etc/apt/sources.list >> "$SOURCES_LIST_FILE" | 150 | cat /etc/apt/sources.list >> "$SOURCES_LIST_FILE" |
44 | 146 | 151 | ||
45 | 147 | echo "Updating apt package lists ..." | 152 | echo "Updating apt package lists ..." |
47 | 148 | sudo apt-get -o "$APT_GET_OPTIONS" update -q | 153 | $sudo apt-get -o "$APT_GET_OPTIONS" update -q |
48 | 149 | 154 | ||
49 | 150 | echo -n "Installing packages ..." | 155 | echo -n "Installing packages ..." |
50 | 151 | 156 | ||
51 | @@ -199,11 +204,11 @@ | |||
52 | 199 | done | 204 | done |
53 | 200 | fi | 205 | fi |
54 | 201 | 206 | ||
56 | 202 | sudo apt-get $FORCE_OPTIONS -o "$APT_GET_OPTIONS" install ${packages} | 207 | $sudo apt-get $FORCE_OPTIONS -o "$APT_GET_OPTIONS" install ${packages} |
57 | 203 | 208 | ||
58 | 204 | if [ "$DEP_PACKAGE_PRESENT" == "yes" ]; then | 209 | if [ "$DEP_PACKAGE_PRESENT" == "yes" ]; then |
59 | 205 | if [ -n "${to_be_installed}" ]; then | 210 | if [ -n "${to_be_installed}" ]; then |
61 | 206 | sudo apt-get $FORCE_OPTIONS -o "$APT_GET_OPTIONS" markauto ${to_be_installed} | 211 | $sudo apt-get $FORCE_OPTIONS -o "$APT_GET_OPTIONS" markauto ${to_be_installed} |
62 | 207 | fi | 212 | fi |
63 | 208 | fi | 213 | fi |
64 | 209 | 214 | ||
65 | 210 | 215 | ||
66 | === modified file 'linaro-media-create' | |||
67 | --- linaro-media-create 2011-01-28 19:50:48 +0000 | |||
68 | +++ linaro-media-create 2011-01-29 19:21:18 +0000 | |||
69 | @@ -22,7 +22,6 @@ | |||
70 | 22 | import os | 22 | import os |
71 | 23 | import sys | 23 | import sys |
72 | 24 | import tempfile | 24 | import tempfile |
73 | 25 | from subprocess import Popen | ||
74 | 26 | 25 | ||
75 | 27 | from linaro_media_create.boards import ( | 26 | from linaro_media_create.boards import ( |
76 | 28 | board_configs, | 27 | board_configs, |
77 | @@ -42,7 +41,10 @@ | |||
78 | 42 | ensure_command, | 41 | ensure_command, |
79 | 43 | is_arm_host, | 42 | is_arm_host, |
80 | 44 | ) | 43 | ) |
82 | 45 | from linaro_media_create import get_args_parser | 44 | from linaro_media_create import ( |
83 | 45 | cmd_runner, | ||
84 | 46 | get_args_parser | ||
85 | 47 | ) | ||
86 | 46 | 48 | ||
87 | 47 | # Just define the global variables | 49 | # Just define the global variables |
88 | 48 | TMP_DIR = None | 50 | TMP_DIR = None |
89 | @@ -60,18 +62,20 @@ | |||
90 | 60 | Before doing so, make sure BOOT_DISK and ROOT_DISK are not mounted. | 62 | Before doing so, make sure BOOT_DISK and ROOT_DISK are not mounted. |
91 | 61 | """ | 63 | """ |
92 | 62 | devnull = open('/dev/null', 'w') | 64 | devnull = open('/dev/null', 'w') |
101 | 63 | # Use raw subprocess.Popen as we don't want to stop in case the | 65 | # ignore non-zero return codes |
102 | 64 | # commands end with a non-zero return code. | 66 | try: |
103 | 65 | if BOOT_DISK is not None: | 67 | if BOOT_DISK is not None: |
104 | 66 | Popen(['sudo', 'umount', BOOT_DISK], | 68 | cmd_runner.Popen(['umount', BOOT_DISK], |
105 | 67 | stdout=devnull, stderr=devnull).wait() | 69 | stdout=devnull, stderr=devnull, as_root=True).wait() |
106 | 68 | if ROOT_DISK is not None: | 70 | if ROOT_DISK is not None: |
107 | 69 | Popen(['sudo', 'umount', ROOT_DISK], | 71 | cmd_runner.Popen(['umount', ROOT_DISK], |
108 | 70 | stdout=devnull, stderr=devnull).wait() | 72 | stdout=devnull, stderr=devnull, as_root=True).wait() |
109 | 73 | except SubcommandNonZeroReturnValue: | ||
110 | 74 | pass | ||
111 | 71 | # Remove TMP_DIR as root because some files written there are | 75 | # Remove TMP_DIR as root because some files written there are |
112 | 72 | # owned by root. | 76 | # owned by root. |
113 | 73 | if TMP_DIR is not None: | 77 | if TMP_DIR is not None: |
115 | 74 | Popen(['sudo', 'rm', '-rf', TMP_DIR]).wait() | 78 | cmd_runner.Popen(['rm', '-rf', TMP_DIR], as_root=True).wait() |
116 | 75 | 79 | ||
117 | 76 | 80 | ||
118 | 77 | def ensure_required_commands(args): | 81 | def ensure_required_commands(args): |
119 | 78 | 82 | ||
120 | === modified file 'linaro_media_create/cmd_runner.py' | |||
121 | --- linaro_media_create/cmd_runner.py 2011-01-28 19:50:48 +0000 | |||
122 | +++ linaro_media_create/cmd_runner.py 2011-01-29 19:21:18 +0000 | |||
123 | @@ -37,10 +37,10 @@ | |||
124 | 37 | """ | 37 | """ |
125 | 38 | assert isinstance(args, (list, tuple)), ( | 38 | assert isinstance(args, (list, tuple)), ( |
126 | 39 | "The command to run must be a list or tuple, found: %s" % type(args)) | 39 | "The command to run must be a list or tuple, found: %s" % type(args)) |
130 | 40 | # TODO: We might want to always use 'sudo -E' here to avoid problems like | 40 | if as_root and os.getuid() != 0: |
128 | 41 | # https://launchpad.net/bugs/673570 | ||
129 | 42 | if as_root: | ||
131 | 43 | args = args[:] | 41 | args = args[:] |
132 | 42 | # TODO: We might want to always use 'sudo -E' here to avoid problems | ||
133 | 43 | # like https://launchpad.net/bugs/673570 | ||
134 | 44 | args.insert(0, 'sudo') | 44 | args.insert(0, 'sudo') |
135 | 45 | return Popen(args, stdin=stdin, stdout=stdout, stderr=stderr) | 45 | return Popen(args, stdin=stdin, stdout=stdout, stderr=stderr) |
136 | 46 | 46 | ||
137 | 47 | 47 | ||
138 | === modified file 'linaro_media_create/tests/test_media_create.py' | |||
139 | --- linaro_media_create/tests/test_media_create.py 2011-01-28 19:50:48 +0000 | |||
140 | +++ linaro_media_create/tests/test_media_create.py 2011-01-29 19:21:18 +0000 | |||
141 | @@ -412,11 +412,18 @@ | |||
142 | 412 | self.assertEqual(0, proc.returncode) | 412 | self.assertEqual(0, proc.returncode) |
143 | 413 | self.assertEqual([['foo', 'bar', 'baz']], fixture.mock.calls) | 413 | self.assertEqual([['foo', 'bar', 'baz']], fixture.mock.calls) |
144 | 414 | 414 | ||
146 | 415 | def test_run_as_root(self): | 415 | def test_run_as_root_with_sudo(self): |
147 | 416 | fixture = self.useFixture(MockCmdRunnerPopenFixture()) | 416 | fixture = self.useFixture(MockCmdRunnerPopenFixture()) |
148 | 417 | self.useFixture(MockSomethingFixture(os, 'getuid', lambda: 1000)) | ||
149 | 417 | cmd_runner.run(['foo', 'bar'], as_root=True).wait() | 418 | cmd_runner.run(['foo', 'bar'], as_root=True).wait() |
150 | 418 | self.assertEqual([['sudo', 'foo', 'bar']], fixture.mock.calls) | 419 | self.assertEqual([['sudo', 'foo', 'bar']], fixture.mock.calls) |
151 | 419 | 420 | ||
152 | 421 | def test_run_as_root_as_root(self): | ||
153 | 422 | fixture = self.useFixture(MockCmdRunnerPopenFixture()) | ||
154 | 423 | self.useFixture(MockSomethingFixture(os, 'getuid', lambda: 0)) | ||
155 | 424 | cmd_runner.run(['foo', 'bar'], as_root=True).wait() | ||
156 | 425 | self.assertEqual([['foo', 'bar']], fixture.mock.calls) | ||
157 | 426 | |||
158 | 420 | def test_run_succeeds_on_zero_return_code(self): | 427 | def test_run_succeeds_on_zero_return_code(self): |
159 | 421 | proc = cmd_runner.run(['true']) | 428 | proc = cmd_runner.run(['true']) |
160 | 422 | # Need to wait() here as we're using the real Popen. | 429 | # Need to wait() here as we're using the real Popen. |
Hi,
I like the change, but it would be good to have a test if possible.
Thanks,
James