Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Rewrite meta/hooks/configure into python3 #24
Conversation
mvo5
added some commits
Mar 9, 2017
|
I added some basic tests, this needs more work, i.e. the mock command, checking output etc. |
mvo5
added some commits
Mar 13, 2017
chipaca
requested changes
Mar 15, 2017
requesting changes for the syncs, and also probably the spurious writing out of the pi conf. The rest are more "did you know" comments, not even nits
| @@ -1,4 +1,4 @@ | ||
| -#!/bin/sh -e | ||
| +#!/usr/bin/python3 |
chipaca
Mar 15, 2017
Member
given you're only importing stdlib things you could add -SIBbb and improve run time a little bit (more an FYI than a request-for-change)
| + | ||
| + | ||
| +def switch_handle_power_key(action): | ||
| + valid_actions = ("ignore","poweroff","reboot","halt","kexec","suspend","hibernate","hybrid-sleep","lock") |
chipaca
Mar 15, 2017
Member
did you know you can now use {} around a listish thing to make it a set? I'd expect to see a set in this kind of lookup thing because of its O(1) behaviour, but obviously given how short it is it'd be a net loss in this case.
chipaca
Mar 15, 2017
Member
(also if you were calling switch_handle_power_key more than once I'd suggest making it a frozenset outside of the funciton, but for a single call the creation time wins, and everything else like readability stays the same, so meh)
| + if not os.path.isdir(login_conf_d): | ||
| + os.makedirs(login_conf_d) | ||
| + if not action in valid_actions: | ||
| + raise Exception("invalid action '{}' supplied for system.power-key-action option".format(action)) |
| + # When the unit is already enabled but not active a call with | ||
| + # --now does start the unit so we have to check for that case | ||
| + # and explicitly start the unit. | ||
| + if systemctl("is-enabled", srv).stdout.strip() == "disabled": |
chipaca
Mar 15, 2017
Member
did you know is-enabled and is-active also set the return value? they have a --quiet option to not print anything if you want to go that route instead
| + if systemctl("is-active", srv).stdout.strip() == "inactive": | ||
| + systemctl("start", srv, check=True) | ||
| + else: | ||
| + raise Exception("Invalid value '{}' provided for option {}".format(enable, srv)) |
| + w.write(line) | ||
| + if not found and value != "": | ||
| + w.write("{}={}\n".format(key, value)) | ||
| + os.rename(path+".tmp", path) |
| + if os.path.isfile(PI_CONFIG): | ||
| + for key in PI_CONFIG_KEYS: | ||
| + value=check_output(["snapctl", "get", "pi-config.{}".format(key.replace("_","-"))], universal_newlines=True).strip() | ||
| + update_pi_config_value(PI_CONFIG, key, value) |
chipaca
Mar 15, 2017
Member
we're writing out the same file multiple times every time, to change a single value in each pass? Am I reading this right?
And we're not syncing any of those writes
This is Not Good
| +def systemctl(*args, check=False): | ||
| + res = subprocess.run(["systemctl"]+list(args), stdout=subprocess.PIPE, stderr=subprocess.PIPE, universal_newlines=True) | ||
| + if check and res.returncode != 0: | ||
| + raise subprocess.CalledProcessError() |
chipaca
and others
added some commits
Mar 15, 2017
| -sync | ||
| +SERVICES=("ssh",) | ||
| + | ||
| +class atomic: |
chipaca
Mar 17, 2017
Member
yes. Is the traceback for when an exception is raised inside a context manager that's done with yield readable now?
|
@mvo5 i'm not actually thrilled by the idea to add an internal python dependency. |
|
@ogra1 I understand the concern. However not having any actual hard dep seems to be not quite true: are what I see from a quick look. Also - this was shell initially, the problems around making |
|
you probably want to re-sync with master to get the missing fixes for the tests |
| @@ -1,4 +1,4 @@ | ||
| -#!/bin/sh -e | ||
| +#!/usr/bin/python3 -SIBbb |
zyga
Apr 4, 2017
Collaborator
Nice, I had to look up what those do but I think it's worth having each one.
zyga
requested changes
Apr 4, 2017
Small review, the context manager code is not correct. I didn't review the rest in detail.
| + fd = os.open(tmp, os.O_WRONLY|os.O_CREAT|os.O_EXCL, mode=0o644, dir_fd=dfd) | ||
| + f = open(fd, "w", closefd=False) | ||
| + | ||
| + yield f |
zyga
Apr 4, 2017
Collaborator
You really have to try/finally to have this behave the way you hope.
try:
...
yield ...
finally:
..
After all the contextmanager decorator is just Python and once an exception flies by you have to have the finally clause to make the cleanups happen.
| + os.close(dfd) | ||
| + | ||
| +def systemctl(*args, check=False): | ||
| + res = subprocess.run(["systemctl"]+list(args), stdout=subprocess.PIPE, stderr=subprocess.PIPE, universal_newlines=True) |
mvo5
Apr 5, 2017
Collaborator
systemctl() is also used for cases when we just need to know the return code and don't want things to blow up, like: if systemctl("is-enabled", "--quiet", srv).returncode != 0: stuff() - this could be done differently or the other way around of course but thats now its done right now :)
| + w.write(line) | ||
| + | ||
| +if __name__ == "__main__": | ||
| + if not core_support_available(): |
zyga
Apr 4, 2017
Collaborator
You may want to wrap that in main() as otherwise each assignment below becomes a global that is visible in other functions.
chipaca
and others
added some commits
Apr 5, 2017
| + dirname, target = os.path.split(path) | ||
| + tmp = target + ".tmp" | ||
| + dfd = os.open(dirname, os.O_RDONLY) | ||
| + fd = os.open(tmp, os.O_WRONLY|os.O_CREAT|os.O_EXCL, mode=0o644, dir_fd=dfd) |
| + dfd = os.open(dirname, os.O_RDONLY) | ||
| + fd = os.open(tmp, os.O_WRONLY|os.O_CREAT|os.O_EXCL, mode=0o644, dir_fd=dfd) | ||
| + | ||
| + f = open(fd, "w", closefd=False) |
chipaca
Apr 5, 2017
Member
os.fdopen is an alias for open, and the docs for os.fdopen don't document closefd (so you need to go read the docs for open anyway)
| + | ||
| + | ||
| +def switch_handle_power_key(action): | ||
| + valid_actions = ("ignore","poweroff","reboot","halt","kexec","suspend","hibernate","hybrid-sleep","lock") |
chipaca
Mar 15, 2017
Member
did you know you can now use {} around a listish thing to make it a set? I'd expect to see a set in this kind of lookup thing because of its O(1) behaviour, but obviously given how short it is it'd be a net loss in this case.
chipaca
Mar 15, 2017
Member
(also if you were calling switch_handle_power_key more than once I'd suggest making it a frozenset outside of the funciton, but for a single call the creation time wins, and everything else like readability stays the same, so meh)
| + if not action in valid_actions: | ||
| + raise Exception("invalid action {!r} supplied for system.power-key-action option".format(action)) | ||
| + with atomic(conf) as fp: | ||
| + fp.write("[Login]\nHandlePowerKey={}\n".format(action)) |
zyga
Apr 5, 2017
Collaborator
You can also just print("[Login]\nHandlePowerKey={}".format(action), file=fp) but not much difference
sergiusens
Apr 5, 2017
@chipaca also prefers your form of this @zyga and I have started to adopt it (without any particular reason)
chipaca
Apr 5, 2017
•
Edited 1 time
-
chipaca
Apr 5, 2017
Member
-
Apr 5, 2017
that chipaca guy sounds really annoying
| + subprocess.check_call(["hooks/configure"]) | ||
| + self.assertEqual(self.mock_systemctl.calls(), [ | ||
| + ["systemctl", "--version"], | ||
| + ["systemctl", "disable", "ssh.service"], |
zyga
Apr 5, 2017
Collaborator
Why didn't we go with the disable --now approach? I recall we had some issues in snapd but my memory is rusty.
| + return MockCmd(self.tmp, basename, script) | ||
| + | ||
| + def mock_snapctl(self, k, v): | ||
| + self.mock_binary("snapctl", """if [ "$1" = "get" ] && [ "$2" = "%s" ]; then echo "%s"; fi""" % (k, v)) |
mvo5
Apr 5, 2017
Collaborator
Yes, would be more correct. We will need to check if k/v are empty because shlex will quote empty values which is not what we need in our script.
| +{1} | ||
| + """.format(self.logfile, script) | ||
| + if not tmpdir in os.environ["PATH"]: | ||
| + os.environ["PATH"] = tmpdir+":"+os.environ["PATH"] |
mvo5
Apr 5, 2017
Collaborator
like this:
- os.environ["PATH"] = tmpdir+":"+os.environ["PATH"]
+ os.environ["PATH"] = os.path.pathsep.join([tmpdir]+os.environ["PATH"].split(os.path.pathsep))
| + content = fp.read() | ||
| + all_calls = [] | ||
| + for call in content.split("\n\n"): | ||
| + if call: |
| + to_write.append("{}={}\n".format(key, config[key])) | ||
| + if needs_write: | ||
| + with atomic(path) as w: | ||
| + for line in to_write: |
|
i am still not convinced python is the right choice here ... one of the main reasons to re-write snapd in go was that it became unusable on single-core/low ram ARM boards when it started to grow. seeing how the hook grew in the recent months and in the light that we seem to start to put gadget configuration in i fear it will also easily grow to a 1000 lines fast and start exposing the same performance issues. |
mvo5
added some commits
Apr 5, 2017
|
As discussed in https://forum.snapcraft.io/t/use-python-for-the-core-snap-configure-hook/168/5 this will be re-written in go instead of python. |
mvo5 commentedMar 13, 2017
During the review of #23 it became apparent that the shell in the configure hook becomes a bit unwieldy and the suggestion to port to python came up.
Below is a 1:1 port from shell to python. If people are happy I can go further and add some helpers and add the missing tests.