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

Fix for https://github.com/NixOS/nixpkgs/pull/258680 #108

Merged
merged 1 commit into from
Jan 21, 2024

Conversation

e-nikolov
Copy link
Contributor

@e-nikolov e-nikolov commented Jan 21, 2024

Fix for NixOS/nixpkgs#258680

network-online.target must now be explicitly depended on

@danderson
Copy link
Member

We shouldn't need to depend on network-online.target at all, that's an antipattern in systemd that slows down boot. tsnet gracefully adapts to network changes on the fly, so it's completely fine to start concurrently with network bringup. I would actually argue for removing the after constraint, rather than adding require.

What problem does this ordering constraint fix? We should fix golink if it doesn't work correctly without this dependency.

Signed-off-by: Emil Nikolov <emil.e.nikolov@gmail.com>
@e-nikolov
Copy link
Contributor Author

I took the safer bet of requiring it because I wasn't sure. Currently golink fails to build on the latest nixpkgs because of the linked PR. I get this error:

error:
       … while checking flake output 'nixosConfigurations'

       … while checking the NixOS configuration 'nixosConfigurations.home-nix'

       … while calling the 'head' builtin

         at /nix/store/kwd6lmx004rkv2r00vj3fcg5ijfvnagk-source/lib/attrsets.nix:960:11:

          959|         || pred here (elemAt values 1) (head values) then
          960|           head values
             |           ^
          961|         else

       (stack trace truncated; use '--show-trace' to show the full trace)

       error:
       Failed assertions:
       - golink.service is ordered after 'network-online.target' but doesn't depend on it
Error: Process completed with exit code 1.

@danderson
Copy link
Member

danderson commented Jan 21, 2024

That... is a weird failure for a different reason, after without wants is a completely legal configuration (ordering constraint on an optional dependency, "if this target is in the boot closure, here's an ordering, but I don't need it"), it's very weird that nixos rejects it.

Let's add the dependency for now to unblock the build, but really we should remove this dependency completely in the longer run. Letting CI run now and I will merge when it passes, thanks!

@danderson danderson merged commit b75983b into tailscale:main Jan 21, 2024
4 checks passed
@e-nikolov
Copy link
Contributor Author

I think they added it as a check when they made "multi-user.target" to no longer depend on "network-online.target" because a lot of existing services depended on that behavior.

@danderson
Copy link
Member

Yeah, fair enough. Ordering without requiring is unusual, especially for network-online.target.

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.

None yet

2 participants