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

Default to systemd, refer to documentation if systemd is not available #336

Merged
merged 1 commit into from
Mar 13, 2023

Conversation

cole-h
Copy link
Member

@cole-h cole-h commented Mar 13, 2023

…able

Description

Closes #308.

Checklist
  • Formatted with cargo fmt
  • Built with nix build
  • Ran flake checks with nix flake check
  • Added or updated relevant tests (leave unchecked if not applicable)
  • Added or updated relevant documentation (leave unchecked if not applicable)
  • Linked to related issues (leave unchecked if not applicable)
Validating with install.determinate.systems

If a maintainer has added the upload to s3 label to this PR, it will become available for installation via install.determinate.systems:

curl --proto '=https' --tlsv1.2 -sSf -L https://install.determinate.systems/nix/pr/$PR_NUMBER | sh -s -- install

We used to default to InitSystem::None if we couldn't detect systemd, but that form of running has some drawbacks (well, one drawback: only root and root-privileged users can use Nix), but that would lead to not obviously-broken installations for users who expected it to work without root.

We now make InitSystem::Systemd the default and error if /run/systemd/system doesn't exist and systemctl does not exist in the PATH (apparently the second may succeed on podman...? cc @Hoverbear you suggested systemd-daemon, but that doesn't exist for me, so I went with the other obvious indication of systemd).

This error suggests --init none, which we used to fall back to, but also links to some (brief) documentation in our README on the drawbacks of installing with InitSystem::None.

@cole-h cole-h self-assigned this Mar 13, 2023
@cole-h cole-h added this to the v0.6.0 milestone Mar 13, 2023
@cole-h cole-h requested a review from Hoverbear March 13, 2023 20:53
@@ -147,6 +147,21 @@ jobs:
run: nix build .
```

## Without systemd (Linux only)
Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe this shouldn't be the second "use case", but somewhat lower?

But also I want this to be visible before the other below use cases that specify --init none... Could just add a link back in place of their "> When --init none is..." notices.

Copy link
Contributor

Choose a reason for hiding this comment

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

Your preference here. I think this is a fine location but am not picky!

Copy link
Member Author

Choose a reason for hiding this comment

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

Myeah, I think this is fine for now. We can always add more docs / more backlinking!

@cole-h cole-h changed the title Default to systemd and refer to documentation if systemd is not avail… Default to systemd, refer to documentation if systemd is not available Mar 13, 2023
@cole-h cole-h merged commit 96d8870 into main Mar 13, 2023
@cole-h cole-h deleted the cole/ds-717-check-systemd-present branch March 13, 2023 21:12
@@ -75,6 +75,13 @@ impl ConfigureInitService {
},
#[cfg(target_os = "linux")]
InitSystem::Systemd => {
// If /run/systemd/system exists, we can be reasonably sure the machine is booted
// with systemd: https://www.freedesktop.org/software/systemd/man/sd_booted.html
if !(Path::new("/run/systemd/system").exists() || which::which("systemctl").is_ok())
Copy link

Choose a reason for hiding this comment

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

Related to #392. On WSL without systemd running, the systemctl command does exist (Ubuntu 22.04.1 from the windows store). So this check would not error, even though systemd needs to be configured in /etc/wsl.conf. It would also be good to point users to the microsoft docs where they document on how to configure this, or maybe even configure the file for people so they only have to reboot.

Note that /run/systemd/system does not exist, so if the check only looked for that it would error succesfully

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for tracking this down!

@@ -75,6 +75,13 @@ impl ConfigureInitService {
},
#[cfg(target_os = "linux")]
InitSystem::Systemd => {
// If /run/systemd/system exists, we can be reasonably sure the machine is booted
// with systemd: https://www.freedesktop.org/software/systemd/man/sd_booted.html
if !(Path::new("/run/systemd/system").exists() || which::which("systemctl").is_ok())
Copy link

Choose a reason for hiding this comment

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

This could be a case of a simple logic error, as in, the following would work:

-if !(Path::new("/run/systemd/system").exists() || which::which("systemctl").is_ok())
---
+if (!Path::new("/run/systemd/system").exists() || !which::which("systemctl").is_ok())

Although I'm not sure what the original intention was

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.

Detect if systemd is present in wsl
3 participants