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/systemd: Temporarily bring back multi-user -> network-online #283818

Conversation

ElvishJerricco
Copy link
Contributor

@ElvishJerricco ElvishJerricco commented Jan 25, 2024

In light of #282795 and #283717, I think it's best to revert this until we've more thoroughly checked that everything is fixed.

Description of changes

The plan is to bring back the undesirable multi-user.target -> network-online.target dependency, but leave in a warning that a unit with an After=network-online.target ordering ought to have a Wants=network-online.target dependency. After we've more thoroughly completed this migration in #282795, we can leave in the warning and remove the multi-user.target -> network-online.target dependency again.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.05 Release Notes (or backporting 23.05 and 23.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@ElvishJerricco ElvishJerricco requested review from Mic92, zowoq, mweinelt and a team as code owners January 25, 2024 19:30
@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: documentation This PR adds or changes documentation 8.has: changelog 8.has: module (update) This PR changes an existing module in `nixos/` labels Jan 25, 2024
Copy link
Member

@lf- lf- left a comment

Choose a reason for hiding this comment

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

Agree

@alyssais
Copy link
Member

Please include the reason for the revert in the commit message.

@ElvishJerricco ElvishJerricco force-pushed the revert-258680-network-online-x-multi-user branch from c128253 to d7d1d4a Compare January 25, 2024 19:33
@ElvishJerricco
Copy link
Contributor Author

@alyssais done

@ElvishJerricco
Copy link
Contributor Author

on second thought, we can just revert smaller bits of this.

@ElvishJerricco ElvishJerricco force-pushed the revert-258680-network-online-x-multi-user branch from d7d1d4a to 07e5ead Compare January 25, 2024 20:31
@ElvishJerricco ElvishJerricco changed the title Revert "Merge pull request #258680 from lf-/jade/remove-multiuser-net… nixos/systemd: Temporarily bring back multi-user -> network-online Jan 25, 2024
@ElvishJerricco ElvishJerricco marked this pull request as ready for review January 25, 2024 20:34
@ElvishJerricco ElvishJerricco requested a review from lf- January 25, 2024 20:35
@ElvishJerricco
Copy link
Contributor Author

Ok, I've redone this to only revert the part that needs to be reverted. All the rest of the original PR is good even without this part.

Copy link
Member

@lf- lf- left a comment

Choose a reason for hiding this comment

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

I think this does the thing.

There were several modules, critically including NetworkManager, which
were not prepared for this change. Most of the change was good,
however. Let's bring back the dependency and change the assertion to a
warning for now.
@ElvishJerricco ElvishJerricco force-pushed the revert-258680-network-online-x-multi-user branch from 07e5ead to 0d85bf0 Compare January 25, 2024 20:53
Copy link
Member

@RaitoBezarius RaitoBezarius left a comment

Choose a reason for hiding this comment

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

LGTM though I think I am only one of those who have been into it for a while, so nothing jump to my eyes.

@delroth delroth added the 12.approvals: 2 This PR was reviewed and approved by two reputable people label Jan 25, 2024
@ElvishJerricco ElvishJerricco merged commit f6d787c into NixOS:master Jan 25, 2024
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: changelog 8.has: documentation This PR adds or changes documentation 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 12.approvals: 2 This PR was reviewed and approved by two reputable people
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants