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

nixos: remove dependencies on local-fs.target #67858

Merged
merged 1 commit into from Sep 2, 2019

Conversation

flokli
Copy link
Contributor

@flokli flokli commented Sep 1, 2019

Since #61321, local-fs.target is
part of sysinit.target again, meaning units without
DefaultDependencies=no will automatically depend on it, and the manual
set dependencies can be dropped.

Motivation for this change

There's still a lot of now-redundant dependencies on local-fs.target in the NixOS module system.

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 nix-review --run "nix-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)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

Copy link
Member

@aanderse aanderse left a comment

Choose a reason for hiding this comment

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

Looking good! 👍

nixos/modules/services/backup/crashplan-small-business.nix Outdated Show resolved Hide resolved
nixos/modules/services/backup/crashplan.nix Outdated Show resolved Hide resolved
@aanderse
Copy link
Member

aanderse commented Sep 1, 2019

@GrahamcOfBorg test transmission

Since NixOS#61321, local-fs.target is
part of sysinit.target again, meaning units without
DefaultDependencies=no will automatically depend on it, and the manual
set dependencies can be dropped.
Copy link
Member

@aanderse aanderse left a comment

Choose a reason for hiding this comment

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

LGTM 👍 We should merge this pretty as soon as we can with the upcoming freeze.

@andir andir merged commit 4e60699 into NixOS:master Sep 2, 2019
dtzWill referenced this pull request in dtzWill/nixpkgs Sep 14, 2019
Since NixOS#61321, local-fs.target is
part of sysinit.target again, meaning units without
DefaultDependencies=no will automatically depend on it, and the manual
set dependencies can be dropped.

(cherry picked from commit f74735c)
@@ -102,7 +102,6 @@ in

systemd.services.triggerhappy = {
wantedBy = [ "multi-user.target" ];
after = [ "local-fs.target" ];
Copy link
Contributor

Choose a reason for hiding this comment

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

Why remove the after clause? Won't this change the semantics?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No - as described in the PR description:

Since #61321, local-fs.target is
part of sysinit.target again, meaning units without
DefaultDependencies=no will automatically depend on it, and the manually set dependencies can be dropped.

triggerhappy.service doesn't set DefaultDependencies=no (ususally only some low-level, mostly systemd-internal services do), so it depends on sysinit.target, which pulls in local-fs.target.

Copy link
Contributor

Choose a reason for hiding this comment

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

But IIUC After= is not about setting dependencies, it only regulates ordering whenever units are started/stopped concurrently. From the manual:

Note that this setting is independent of and orthogonal to the requirement dependencies as configured by Requires=, Wants= or BindsTo=

@flokli flokli deleted the local-fs-target-services branch September 16, 2019 15:03
@flokli
Copy link
Contributor Author

flokli commented Sep 16, 2019 via email

@asymmetric
Copy link
Contributor

Thank you, that makes sense now!

Mic92 added a commit to Mic92/nixpkgs that referenced this pull request Sep 23, 2019
This change was re-introduced when updating to systemd 243.
Also see: NixOS#67858

(cherry picked from commit 53fb1c5)
Mic92 added a commit that referenced this pull request Sep 23, 2019
This change was re-introduced when updating to systemd 243.
Also see: #67858
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants