Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

mu: run initialization command when personal addresses change #6100

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

VojtechStep
Copy link

@VojtechStep VojtechStep commented Nov 18, 2024

Description

When the user changes which addresses mu should consider 'personal', mu's store should be reinitialized.

After this change, the activation script parses the previously configured list of addresses and compares it with the new one. If they differ, it runs the init command even when the store has already been initialized.

Checklist

  • Change is backwards compatible.

  • Code formatted with ./format.

  • Code tested through nix-shell --pure tests -A run.all or nix develop --ignore-environment .#all using Flakes.

  • Test cases updated/added. See example.

  • Commit messages are formatted like

    {component}: {description}
    
    {long description}
    

    See CONTRIBUTING for more information and recent commit messages for examples.

  • If this PR adds a new module

    • Added myself as module maintainer. See example.

Maintainer CC

@KarlJoad

@VojtechStep VojtechStep marked this pull request as draft November 19, 2024 12:20
@VojtechStep VojtechStep force-pushed the feature/mu-personal-addresses branch from 83b89fa to fd326d8 Compare November 19, 2024 14:27
@VojtechStep VojtechStep marked this pull request as ready for review November 19, 2024 14:29
@VojtechStep VojtechStep force-pushed the feature/mu-personal-addresses branch from fd326d8 to 61873f8 Compare December 9, 2024 17:20
@KarlJoad
Copy link
Contributor

KarlJoad commented Dec 9, 2024

This looks interesting! I have long tinkered with this kind of thing in my head. Does this also handle the case where you add/remove only some addresses/aliases? I would like to codify the expected behavior of changing a subset of the addresses as an actual test that Nix/home-manager can run if we do go down this road.

@VojtechStep
Copy link
Author

Does this also handle the case where you add/remove only some addresses/aliases?

On activation it asks mu for the current list of aliases, so any observable change to that (modulo order) should trigger a reinit.

I would like to codify the expected behavior of changing a subset of the addresses as an actual test that Nix/home-manager can run if we do go down this road.

I agree, though it would involve testing the actual side-effects of activation over multiple runs, and I don't know if the current test infra allows that.

@VojtechStep
Copy link
Author

I agree, though it would involve testing the actual side-effects of activation over multiple runs, and I don't know if the current test infra allows that.

I tried to get the integration tests working, but I can't get a working internet connection in the VMs. In any case I'm pretty sure the patch works, since I've been using it for the past couple weeks, during which I manipulated my email addresses (adding new aliases and accounts), and mu always ended up in the expected state.

@teto
Copy link
Collaborator

teto commented Dec 12, 2024

willing to merge but can't review. @KarlJoad could you test/review and confirm it works ?

@KarlJoad
Copy link
Contributor

I can review, but cannot test on real-world stuff. I do not use home-manager anymore and I don't want to impact my current mail setup.

I will post my review/approve by tonight CST.

@teto teto added the mail label Dec 12, 2024
@VojtechStep
Copy link
Author

I'm not sure if I got the usage of run in the activation script right. On dry runs, should the check for currently registered email addresses run, so that the log correctly reflects if reinit would run or not, or should the check also not run?

@KarlJoad
Copy link
Contributor

In my opinion, a dry-run should do everything a normal run should do, except for any actual modifications, that way you (as the user) can see exactly what will happen, without anything actually changing.

So dry-runs should check for currently-registered addresses.

@VojtechStep VojtechStep force-pushed the feature/mu-personal-addresses branch from 61873f8 to cab44ac Compare December 13, 2024 21:38
@github-actions github-actions bot removed the mail label Dec 13, 2024
@VojtechStep
Copy link
Author

So dry-runs should check for currently-registered addresses.

I agree; fixed the check in dry-run mode and rebased on master.

When the user changes which addresses mu should consider 'personal',
mu's store should be reinitialized.

After this change, the activation script parses the previously
configured list of addresses and compares it with the new one. If they
differ, it runs the init command even when the store has already been
initialized.
@VojtechStep VojtechStep force-pushed the feature/mu-personal-addresses branch from cab44ac to d5ce971 Compare December 16, 2024 12:39
@VojtechStep
Copy link
Author

This is hopefully the last change, sorry. I'm setting up a new device today, and the current address detection failed because the store has never been initialized.

@KarlJoad
Copy link
Contributor

KarlJoad commented Dec 16, 2024

No worries! I'm glad you caught this one.

Copy link
Contributor

@KarlJoad KarlJoad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the current address detection failed because the store has never been initialized.

Honestly, I would have expected this to be caught by a test. Could you add a test for this too?

@VojtechStep
Copy link
Author

Honestly, I would have expected this to be caught by a test.

Yeah, but it needs to be an integration test, and I still haven't managed to run those

@VojtechStep VojtechStep force-pushed the feature/mu-personal-addresses branch from 7c0a10f to 5790ca9 Compare December 17, 2024 16:36
@VojtechStep
Copy link
Author

I figured out how to run integration tests (in hindsight it makes sense that the VM can't access the internet when it's running in sandbox, so I had to turn that off; also I was getting error: path /home/alice/.nix-defexpr/channels/home-manager is a symlink because home.revision tries to read from it, but I didn't dig further, just set the default to null).

Hopefully now it should be mergeable.

@KarlJoad
Copy link
Contributor

I don't seem to be able to run the test locally, for some reason (Podman on my desktop). What is the exact run.?? that I can use on the command-line to run these integration tests?

If I get this to test successfully locally, I'll gladly approve this again!

Thanks for all your hard work on this @VojtechStep.

@VojtechStep
Copy link
Author

What is the exact run.?? that I can use on the command-line to run these integration tests?

With flakes it's nix build -L .#integration-test-mu, otherwise nix-build --arg pkgs 'import <nixpkgs> {}' tests/integration -A mu, since AFAICT integration tests are not exposed in tests/default.nix (the explicit pkgs instantiation is necessary because it doesn't have a default value). If you get a couldn't download ... error in the VM then you need to turn off the sandbox, and if you get a path ... is a symlink error, then change modules/misc/version.nix:78 from default = let ... to default = null; (I don't know why that's broken now, but all standalone integration tests are broken by it).

@KarlJoad
Copy link
Contributor

I actually ended up with a different error, which is probably just operator error.

$ nix build -L .#integration-test-mu --no-sandbox
...
machine # building '/nix/store/xjkydxc0n24mwxp8kh4wn5jq0fppga9k-bootstrap-tools.tar.xz.drv'...
machine # warning: error: unable to download 'http://tarballs.nixos.org/stdenv/x86_64-unknown-linux-gnu/82b583ba2ba2e5706b35dbe23f31362e62be2a9d/bootstrap-tools.tar.xz': Could not resolve hostname (6); retrying in 258 ms
machine # warning: error: unable to download 'http://tarballs.nixos.org/stdenv/x86_64-unknown-linux-gnu/82b583ba2ba2e5706b35dbe23f31362e62be2a9d/bootstrap-tools.tar.xz': Could not resolve hostname (6); retrying in 591 ms
machine # warning: error: unable to download 'http://tarballs.nixos.org/stdenv/x86_64-unknown-linux-gnu/82b583ba2ba2e5706b35dbe23f31362e62be2a9d/bootstrap-tools.tar.xz': Could not resolve hostname (6); retrying in 1174 ms
machine # warning: error: unable to download 'http://tarballs.nixos.org/stdenv/x86_64-unknown-linux-gnu/82b583ba2ba2e5706b35dbe23f31362e62be2a9d/bootstrap-tools.tar.xz': Could not resolve hostname (6); retrying in 2383 ms
machine # error:
machine #        … writing file '/nix/store/2pizl7lq4awa7p9bklr8037yh1sca0hg-bootstrap-tools.tar.xz'
machine #
machine #        error: unable to download 'http://tarballs.nixos.org/stdenv/x86_64-unknown-linux-gnu/82b583ba2ba2e5706b35dbe23f31362e62be2a9d/bootstrap-tools.tar.xz': Could not resolve hostname (6)
machine # error: builder for '/nix/store/xjkydxc0n24mwxp8kh4wn5jq0fppga9k-bootstrap-tools.tar.xz.drv' failed with exit code 1;
machine #        last 4 log lines:
machine #        > error:
machine #        >        … writing file '/nix/store/2pizl7lq4awa7p9bklr8037yh1sca0hg-bootstrap-tools.tar.xz'
machine #        >
machine #        >        error: unable to download 'http://tarballs.nixos.org/stdenv/x86_64-unknown-linux-gnu/82b583ba2ba2e5706b35dbe23f31362e62be2a9d/bootstrap-tools.tar.xz': Could not resolve hostname (6)
machine #        For full logs, run 'nix-store -l /nix/store/xjkydxc0n24mwxp8kh4wn5jq0fppga9k-bootstrap-tools.tar.xz.drv'.
machine # error: 1 dependencies of derivation '/nix/store/05q48dcd4lgk4vh7wyk330gr2fr082i2-bootstrap-tools.drv' failed to build
machine # error: 1 dependencies of derivation '/nix/store/s63zivn27i8qv5cqiy8r5hf48r323qwa-bash-5.2p37.drv' failed to build
machine # building '/nix/store/026i5h3a3pg28dxgv1x90117z6diha3x-byacc-20240109.tgz.drv'...
machine # error: 1 dependencies of derivation '/nix/store/by983vw5x0b8i6cbcz5y2hkd0n3n4a9b-coreutils-9.5.drv' failed to build
machine # error: 1 dependencies of derivation '/nix/store/zrp1z93l9cj9ia4n6s9cfs0f3s37r8v1-gcc-wrapper-13.3.0.drv' failed to build
machine # error: 1 dependencies of derivation '/nix/store/qhdvi3qcn60vrapyhsxxpbw0q63gmfz8-glibc-2.40-36.drv' failed to build
machine # error: 1 dependencies of derivation '/nix/store/vxckchzd4ny3dni980qf570fmfc3q5m6-stdenv-linux.drv' failed to build
machine # error: 1 dependencies of derivation '/nix/store/ws0w1jw9y3wwmvm305m18ivjil4nayls-stdenv-linux.drv' failed to build
machine # error: build of '/nix/store/7samcjpvkdln5jk24i3mmfl8blxnjmw8-home-manager.drv', '/nix/store/ws0w1jw9y3wwmvm305m18ivjil4nayls-stdenv-linux.drv' failed
machine # [  111.963817] su[1025]: pam_unix(su:session): session closed for user alice
machine: output:
cleanup
kill machine (pid 9)
qemu-system-x86_64: terminating on signal 15 from pid 6 (/nix/store/zv1kaq7f1q20x62kbjv6pfjygw5jmwl6-python3-3.12.7/bin/python3.12)
kill vlan (pid 7)
(finished: cleanup, in 0.00 seconds)
Traceback (most recent call last):
  File "/nix/store/ylz95zwxwfihzfwyv5pylkf92hn1yxd6-nixos-test-driver-1.1/bin/.nixos-test-driver-wrapped", line 9, in <module>
    sys.exit(main())
             ^^^^^^
  File "/nix/store/ylz95zwxwfihzfwyv5pylkf92hn1yxd6-nixos-test-driver-1.1/lib/python3.12/site-packages/test_driver/__init__.py", line 146, in main
    driver.run_tests()
  File "/nix/store/ylz95zwxwfihzfwyv5pylkf92hn1yxd6-nixos-test-driver-1.1/lib/python3.12/site-packages/test_driver/driver.py", line 174, in run_tests
    self.test_script()
  File "/nix/store/ylz95zwxwfihzfwyv5pylkf92hn1yxd6-nixos-test-driver-1.1/lib/python3.12/site-packages/test_driver/driver.py", line 166, in test_script
    exec(self.tests, symbols, None)
  File "<string>", line 41, in <module>
  File "<string>", line 22, in succeed_as_alice
  File "/nix/store/ylz95zwxwfihzfwyv5pylkf92hn1yxd6-nixos-test-driver-1.1/lib/python3.12/site-packages/test_driver/machine.py", line 612, in succeed
    raise Exception(f"command `{command}` failed (exit code {status})")
Exception: command `su -l alice --shell /bin/sh -c $'export XDG_RUNTIME_DIR=/run/user/$UID ; nix-shell "<home-manager>" -A install'` failed (exit code 1)

@VojtechStep
Copy link
Author

I tried that to disable the sandbox at first, but got a warning that I'm not a trusted user, so the setting is ignored. I ended up temporarily disabling it from my NixOS config. Could something similar be happening to you?

@KarlJoad
Copy link
Contributor

It could be. I have not added my user to /etc/nix.conf as a trustedUser. I can try adding my user to that list, but it will take me a while. I have not updated my computer in a while, and I have been planning to combine an upgrade with my next reconfigure.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants