[WIP] Port switch-to-configuration script to Python #48378

bgamari · 2018-10-14T06:17:11Z

Motivation for this change

Note that this has yet to be tested. Rather I am just posting this as a placeholder.

This is a port of the switch-to-configuration script from Perl to Python. The motivation for this change is to ease cross-compilation, which is documented to be quite difficult in the case of Perl (see #36675).

One tricky issue that this raises is the lack of sensible dbus bindings for Python. Each of the options has its share of issues:

dbus-python is largely unmaintained and binds the libdbus implementation, use of which is somewhat actively discouraged
pydbus uses gobject-introspection, which sadly appears to be very unfriendly to cross-compilation
QtDBus uses Qt, which is rather heavy-weight for this purpose.

Ideally I would like a small binding for sdbus but, that not being available, I have simply used dbus-python here.

Things done

Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
Built on platform(s)
- NixOS
- macOS
- other Linux distributions
Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
Tested execution of all binary files (usually in ./result/bin/)
Determined the impact on package closure size (by running nix path-info -S before and after)
Fits CONTRIBUTING.md.

bgamari · 2018-10-14T06:21:34Z

CCing @shlevy @Ericson2314

FRidh

Move the main code flow into main() and add

if __name__ == "__main__": 
    main()

import statements should all be at the top. Please also add docstrings.

Mic92 · 2018-10-14T07:37:15Z

nixos/modules/system/activation/switch-to-configuration.py

+restartListFile = "/run/systemd/restart-list"
+reloadListFile = "/run/systemd/reload-list"
+
+def die_with_usage():


Would you mind to type annotations, then changing this file becomes easier for type checking with mypy.
I can also add them if you want.

I was very tempted to add type annotations when I started but wasn't quite sure whether we would want to retain Python 2 support so (mostly) refrained. I would be happy to add them.

No, we definitely don't want more legacy python2 code.

FRidh · 2018-10-14T07:26:11Z

nixos/modules/system/activation/switch-to-configuration.py

+reloadListFile = "/run/systemd/reload-list"
+
+def die_with_usage():
+    print("""


consider a function error which calls print with file=stderr
https://stackoverflow.com/questions/5574702/how-to-print-to-stderr-in-python

FRidh · 2018-10-14T07:27:17Z

nixos/modules/system/activation/switch-to-configuration.py

+    else:
+        return otherwise
+
+if len(sys.argv) < 1:


use argparse for building the parser. Call it from main().

FRidh · 2018-10-14T07:27:41Z

nixos/modules/system/activation/switch-to-configuration.py

+out = "@out@"
+
+# To be robust against interruption, record what units need to be started etc.
+startListFile = "/run/systemd/start-list"


constants should be in uppercase

FRidh · 2018-10-14T07:28:59Z

nixos/modules/system/activation/switch-to-configuration.py

+
+# Install or update the bootloader.
+if action == "switch" or action == "boot":
+    subprocess.checkCall(["@installBootLoader@", out])


FRidh · 2018-10-14T07:29:27Z

nixos/modules/system/activation/switch-to-configuration.py

+
+# Just in case the new configuration hangs the system, do a sync now.
+if os.environ.get('NIXOS_NO_SYNC') != "1":
+    subprocess.checkCall(["@coreutils@/bin/sync", "-f", "/nix/store"])


FRidh · 2018-10-14T07:34:00Z

nixos/modules/system/activation/switch-to-configuration.py

+# absolute path as well.
+def fingerprint_unit(filename):
+    fprint = os.path.abspath(filename)
+    override = '%s.d/overrides.conf' % filename


use .format

or python3.6 format strings

Are we ready to accept a >= Python 3.6 as a requirement for NixOS?

Yes. python36 is the default python3, and soon it will be python37.

FRidh · 2018-10-14T07:35:25Z

nixos/modules/system/activation/switch-to-configuration.py

+        if not os.path.exists(prev_unit_file) \
+           and not os.path.exists(new_unit_file) \
+           and m is not None
+            baseUnit = "%s@%s" % (m.group(1), m.group(1))


underscores

Good catch!

FRidh · 2018-10-14T07:36:19Z

nixos/modules/system/activation/switch-to-configuration.py

+    transitions = UnitTransitions()
+    active_prev = get_active_units()
+
+    for unit, state in active_prev.items():


I think what's below this for loop should be in a separate function

FRidh · 2018-10-14T07:36:47Z

nixos/modules/system/activation/switch-to-configuration.py

+        prev_exists = os.path.exists(prev_unit_file)
+        new_exists = os.path.exists(new_unit_file)
+        if prev_exists and state['state'] in ['active', 'activating']:
+            if (not new_exists) or os.path.abspath(new_unit_file) == "/dev/null":


too much nested. Split up in functions.

FRidh · 2018-10-14T07:39:58Z

nixos/modules/system/activation/switch-to-configuration.py

+    output = subprocess.check_output(args)
+    return output.strip()
+
+def unique(*args):


Mic92 · 2018-10-14T07:40:50Z

nixos/modules/system/activation/switch-to-configuration.py

+if len(sys.argv) < 1:
+    die_with_usage()
+
+action = sys.argv[1]


Can you add those lines in a one main function instead of mixing functions and code?

FRidh · 2018-10-14T07:46:06Z

@Mic92 I think we can add a tiny builder in trivial-builders for building a Python script, that is also being checked with mypy and pylint. We may want to have the option for disabling those checks.

Mic92 · 2018-10-14T07:48:17Z

There is also pystemd: https://github.com/facebookincubator/pystemd that wraps around libsystemd,
but I don't know what the story of cross-compiling cython is at the moment. It should be lightweight at least.
cc @flokli #47517

Mic92 · 2018-10-14T07:54:30Z

@FRidh I also have some other scripts flying around that can profit from mypy builders: https://github.com/NixOS/nixpkgs/blob/master/pkgs/misc/vim-plugins/update.py https://github.com/NixOS/nixpkgs/blob/master/pkgs/development/tools/analysis/radare2/update.py

Mic92 · 2018-10-14T07:56:35Z

I also started rewriting setup-etc.pl at some point, but I think I lost the source code for that.

Mic92 · 2018-10-14T08:00:13Z

nixos/modules/system/activation/switch-to-configuration.py

+
+                    if reload_if_changed:
+                        transitions.reload_unit(unit)
+                    elif !restart_if_changed or refuse_manual_stop:


This is not valid python syntax.

Mic92 · 2018-10-14T08:00:22Z

nixos/modules/system/activation/switch-to-configuration.py

+                    elif !restart_if_changed or refuse_manual_stop:
+                        transitions.skip_unit(unit)
+                    else:
+                        if !stop_if_changed:


also this one should be not.

arianvp · 2018-10-14T08:13:52Z

Pystemd would indeed be very nice for this usecase. @flokli and I have been experimenting with it and it seemed to work really well. I'll have a look at Cython's cross-compilation story. Because I'm not sure either.

jtojnar · 2018-10-14T08:36:19Z

What about using Jeepney for DBus? It is a pure Python library.

bgamari · 2018-10-14T08:39:33Z

What about using Jeepney for DBus? It is a pure Python library.

I considered it but decided against it for now for concerns of correctness and verbosity. I'd be happy to go this direction if others think it best, however.

bgamari · 2018-10-14T08:41:05Z

The current head typechecks with mypy.

Mic92 · 2018-10-14T08:47:03Z

nixos/modules/system/activation/switch-to-configuration.py

+    print_err(msg)
+    sys.exit(1)
+
+def die(why: str):


Functions should also always have a return type also if it its None. Otherwise the type check is skipped if a function does not have any typed arguments.

Mic92 · 2018-10-14T08:47:23Z

nixos/modules/system/activation/switch-to-configuration.py

+
+# This is a NixOS installation if it has /etc/NIXOS or a proper
+# /etc/os-release.
+def is_nixos():


def is_nixos() -> bool:

pkgs/development/tools/build-managers/meson/default.nix

The build process relies on the glib-marshal utility.

Strangely, they were previously marked as nativeBuildInputs.

bgamari · 2018-10-14T17:19:17Z

@Mic92, this now includes a port of setup-etc as well (although it might be wise to split this into a new PR)

nixos/modules/system/activation/switch-to-configuration.py

FRidh · 2018-11-01T10:50:27Z

FWIW, see #48611 regarding nfs-utils.

bgamari · 2018-11-01T15:29:34Z

Alright, I will put this aside then. Thanks to everyone who reviewed!

edolstra · 2018-11-01T15:53:39Z

The only Python script in NixOS is systemd-boot-builder.py, which kind of slipped in under the radar. I once started to rewrite it in Perl, but didn't finish it.

edolstra · 2018-11-01T15:55:42Z

So that one script effectively increases closure sizes by 55 MiB.

grahamc · 2018-11-01T18:16:34Z

@FRidh I just bisected to that very commit. Tough :/

andrew-d · 2018-11-01T22:16:12Z

@edolstra - OOC, is the primary concern closure size? Any thoughts on a more acceptable number (e.g. is a 5MB increase okay)?

bgamari · 2018-11-01T22:19:27Z

@edolstra,j while I'm not attached to Python in particular, I really don't see the sense in using Perl over Python. If anything it seems to me that the residual Perl should be moved to Python, not the other way around. My reasons are three-fold:

Python has a sensible static analysis/typechecking story, which Perl (AFAIK) does lacks. I have been pretty impressed by how much mypy can catch.
Perl is, frankly, hell to cross-compile and very little can be done to fix this. The language and its ecosystem were simply developed with very little consideration for cross-compilation.
In 2018 it is much easier to find people to work in Python than Perl

A compiled language (Rust or C++) also would be fine here. Really, just anything but Perl.

edolstra · 2018-11-01T22:51:09Z

@andrew-d No, the main concern is not doing gratuitous rewrites. The motivation (cross-compilation) is a weak one because NixOS does not support cross-compilation.

Also, I find Python a horrible language to maintain (worse than Perl, even). Rewriting NixOps in Python was the worst decision I ever made. Never again.

edolstra · 2018-11-01T23:06:01Z

rust/go binaries would be even larger in size then scripts

A Rust binary would not be bigger than the 50+ MiB that Python adds to the closure size. All the current Perl scripts (switch-to-configuration.pl, setup-etc.pl, install-grub.pl, nixos-generate-config.pl, update-users-groups.pl, command-not-found.pl) could be replaced by a single Rust binary that shouldn't be bigger than a megabyte or two.

bgamari · 2018-11-02T01:46:09Z

No, the main concern is not doing gratuitous rewrites. The motivation (cross-compilation) is a weak one because NixOS does not support cross-compilation.

I'm not sure what you mean by this; Both @Ericson2314 and I have put quite a bit of work into fixing cross-compilation in both Nixpkgs and NixOS. However, at this point I cross-compile NixOS routinely and use it to support a client project.

Ericson2314 · 2018-11-02T05:14:20Z

I give others credit for NixOS in particular :). But yeah as I understand it the only obstical remaining is Perl.

I don't love either but Python > Perl is also by far the more common opinion as others have said, and that should be taken into account.

I don't think Rust buys us much either. I'm a huge believer in static types, but if everything but the dbus part is shelling out, it's still fundamentally stringly typed. Hardening non-weakest links is a rather pointless exercise.

Still, if Rust is the best way for this to move forward, so be it. I haven't looked that much in the nixpkgs some of things, but in principle it should work very well.

edolstra · 2018-11-05T21:56:49Z

Does that actually work at the moment? The shebang paths in switch-to-configuration and other scripts in 64-bit closures won't work on 32-bit systems either.

To be honest I don't think it's a use case that needs to be a hard requirement.

Mic92 · 2018-11-28T18:04:17Z

Rust would be soon usable w.r.t to cross compiling: #50866

zimbatm · 2018-11-28T18:24:38Z

We can also rewrite those scripts in Bash, bash has a small closure size ;)

Mic92 · 2018-11-29T10:39:23Z

They were in bash before, but where rewritten in perl because they were too slow according to the git history.

bgamari · 2018-12-07T17:26:35Z

Concerning

It would be nice to rewrite stdenv builders from bash to perl

I would just like to repeat my previous point:

Perl is, frankly, hell to cross-compile and very little can be done to fix this. The language and its ecosystem were simply developed with very little consideration for cross-compilation.

I would hope that this disqualifies the language from any use in something as central as the stdenv builders.

danbst · 2019-01-02T09:21:42Z

@edolstra

Also, I find Python a horrible language to maintain (worse than Perl, even). Rewriting NixOps in Python was the worst decision I ever made. Never again.

This is unique experience. Can you write-up a separate post on "why"? Or is it more personal, so nothing to share?

rnhmjoj · 2021-01-07T22:35:55Z

This is unique experience. Can you write-up a separate post on "why"? Or is it more personal, so nothing to share?

If I had to guess, breaking changes in the standard library between major/minor releases.
Perl is really stable: you can take 25yo script and it will still run unchanged.

kvtb · 2021-01-10T19:57:29Z

Besides increasing minimal system closure size, switching to python would increase number of packets required to cross-compile a system closure: Python + some libs + deps would need to be cross-compilable.

I nice idea I think is to rewrite switch-to-configuration in C++ and build as i686 static executable.
Although it might put the readability even beyond perl, it would:

decrease minimal system closure size
allow to nix-rebuild boot; reboot (and to nix-infect) x64_86 closures on top of i686 installations effectively upgrading them to 64-bit. Currently switch-to-configuration in x64_86 closure requires 64-bit perl which does not run on i686, so nix-rebuild boot; reboot works only in one direction (x64_86 -> i686 using 32-bit perl which runs on both). switch-to-configuration in C++ would enable both directions.

kvtb · 2021-01-10T20:00:16Z

@edolstra

Also, I find Python a horrible language to maintain (worse than Perl, even). Rewriting NixOps in Python was the worst decision I ever made. Never again.

This is unique experience. Can you write-up a separate post on "why"? Or is it more personal, so nothing to share?

That raises questions how the decision on test-driver perl->python rewrite had been passed and why @edolstra did not vetoed it (I agree that as embed language python is terrible + having tab-language within Nix string literals with non-trivial indent stripping and escape sequences is awful)

Mic92 · 2021-01-10T20:52:10Z

@edolstra

Also, I find Python a horrible language to maintain (worse than Perl, even). Rewriting NixOps in Python was the worst decision I ever made. Never again.

This is unique experience. Can you write-up a separate post on "why"? Or is it more personal, so nothing to share?

That raises questions how the decision on test-driver perl->python rewrite had been passed and why @edolstra did not vetoed it (I agree that as embed language python is terrible + having tab-language within Nix string literals with non-trivial indent stripping and escape sequences is awful)

Because not many contributors know perl anymore.

flokli · 2021-01-10T21:01:38Z

Also note perl is probably worse when it comes to cross-compilation than python.

kvtb · 2021-01-10T21:08:33Z

Also note perl is probably worse when it comes to cross-compilation than python.

Yes, we depend on https://arsv.github.io/perl-cross/ which is not very reliable. The project is not very popular and they might stop supporting it at any moment. That's +1 for C++

meson: fix cross compilation of systemd

787b44f

GrahamcOfBorg added 6.topic: nixos 8.has: module (update) 10.rebuild-darwin: 501+ 10.rebuild-linux: 501+ labels Oct 14, 2018

FRidh requested changes Oct 14, 2018

View reviewed changes

Mic92 reviewed Oct 14, 2018

View reviewed changes

FRidh requested changes Oct 14, 2018

View reviewed changes

Mic92 reviewed Oct 14, 2018

View reviewed changes

bgamari force-pushed the nixos-cross-fixes branch from 5dcbc76 to 02ce1c6 Compare October 14, 2018 08:34

Mic92 reviewed Oct 14, 2018

View reviewed changes

FRidh reviewed Oct 14, 2018

View reviewed changes

pkgs/development/tools/build-managers/meson/default.nix Show resolved Hide resolved

bgamari added 2 commits October 14, 2018 13:02

dbus-glib: Add glib as nativeBuildInput

67ca9c1

The build process relies on the glib-marshal utility.

shared-mime-info: libxml2 and glib should be buildInputs

a607dcf

Strangely, they were previously marked as nativeBuildInputs.

bgamari force-pushed the nixos-cross-fixes branch from 13699b1 to bcc9a6b Compare October 14, 2018 17:18

symphorien reviewed Oct 14, 2018

View reviewed changes

nixos/modules/system/activation/switch-to-configuration.py Show resolved Hide resolved

symphorien reviewed Oct 14, 2018

View reviewed changes

nixos/modules/system/activation/switch-to-configuration.py Outdated Show resolved Hide resolved

Mic92 mentioned this pull request Nov 2, 2018

Check types wit mypy NixOS/nixops#1039

Merged

jtojnar mentioned this pull request Nov 29, 2018

Remove perl dependency in activation scripts #51198

Open

adisbladis mentioned this pull request Aug 17, 2019

python3Minimal: Add top-level for a minimal Python 3 build #66762

Merged

10 tasks

Mic92 mentioned this pull request Dec 3, 2019

command-not-found: drop perl dependency #74789

Closed

10 tasks

Enteee mentioned this pull request Mar 17, 2020

update-users-groups.pl: use atomic write_file #82755

Closed

12 tasks

Mic92 mentioned this pull request Jun 23, 2020

nixos-generate-config: add Rust stub with tests #91353

Closed

xaverdh mentioned this pull request Aug 21, 2020

nixos/grub: generate BLS entries #95901

Draft

10 tasks

[WIP] Port switch-to-configuration script to Python #48378

[WIP] Port switch-to-configuration script to Python #48378

Conversation

bgamari commented Oct 14, 2018 • edited

Motivation for this change

Things done

bgamari commented Oct 14, 2018

FRidh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FRidh commented Oct 14, 2018

Mic92 commented Oct 14, 2018 • edited

Mic92 commented Oct 14, 2018 • edited

Mic92 commented Oct 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arianvp commented Oct 14, 2018

jtojnar commented Oct 14, 2018

bgamari commented Oct 14, 2018 • edited

bgamari commented Oct 14, 2018

Mic92 Oct 14, 2018 • edited

Choose a reason for hiding this comment

Mic92 Oct 14, 2018 • edited

Choose a reason for hiding this comment

bgamari commented Oct 14, 2018

FRidh commented Nov 1, 2018

bgamari commented Nov 1, 2018

edolstra commented Nov 1, 2018

edolstra commented Nov 1, 2018

grahamc commented Nov 1, 2018

andrew-d commented Nov 1, 2018

bgamari commented Nov 1, 2018

edolstra commented Nov 1, 2018

edolstra commented Nov 1, 2018

bgamari commented Nov 2, 2018

Ericson2314 commented Nov 2, 2018

edolstra commented Nov 5, 2018

Mic92 commented Nov 28, 2018

zimbatm commented Nov 28, 2018

Mic92 commented Nov 29, 2018

bgamari commented Dec 7, 2018

danbst commented Jan 2, 2019

rnhmjoj commented Jan 7, 2021

kvtb commented Jan 10, 2021 • edited

kvtb commented Jan 10, 2021 • edited

Mic92 commented Jan 10, 2021

flokli commented Jan 10, 2021

kvtb commented Jan 10, 2021

bgamari commented Oct 14, 2018 •

edited

Mic92 commented Oct 14, 2018 •

edited

Mic92 commented Oct 14, 2018 •

edited

bgamari commented Oct 14, 2018 •

edited

Mic92 Oct 14, 2018 •

edited

Mic92 Oct 14, 2018 •

edited

kvtb commented Jan 10, 2021 •

edited

kvtb commented Jan 10, 2021 •

edited