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.

Sign up for GitHub

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

Jump to bottom

Revert "Add networking.hosts and .hostFiles from nixos " #1353

Merged
merged 2 commits into from
Feb 26, 2025
Merged

Revert "Add networking.hosts and .hostFiles from nixos " #1353

merged 2 commits into from
Feb 26, 2025

Conversation

booxter
Copy link
Contributor

@booxter booxter commented Feb 26, 2025

Reverts #939

@booxter
Copy link
Contributor Author

booxter commented Feb 26, 2025

Testing if this solves my issues with localhost resolution for MacOS ssh client.

@booxter booxter marked this pull request as ready for review February 26, 2025 14:27
@booxter
Copy link
Contributor Author

booxter commented Feb 26, 2025 •
edited
Loading

Yes, it does resolve my issues. With the offending patch:

$ /usr/bin/ssh localhost -p 31022
ssh: Could not resolve hostname localhost: nodename nor servname provided, or not known

With this revert:

$ /usr/bin/ssh localhost -p 31022
The authenticity of host '[localhost]:31022 ([127.0.0.1]:31022)' can't be established.
...

@booxter booxter mentioned this pull request Feb 26, 2025
Open
emilazy
emilazy previously approved these changes Feb 26, 2025
View reviewed changes
Copy link
Collaborator

@emilazy emilazy left a comment

Choose a reason for hiding this comment

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

Seems best to do this for now given the issues people are running into, but cc @Enzime.

@booxter
Copy link
Contributor Author

booxter commented Feb 26, 2025

Oh we may now have to restore the original file that was moved to /etc/hosts.before-nix-darwin, or handle the recovery in some form.

@emilazy
Copy link
Collaborator

emilazy commented Feb 26, 2025

Right. You can add mv /etc/hosts{.before-nix-darwin,} to an activation script.

@booxter
Copy link
Contributor Author

booxter commented Feb 26, 2025

FYI I may not get to implementing an activation script in the next ~8h so if someone would like to take this over, be my guest. 🙏

Enzime
Enzime previously approved these changes Feb 26, 2025
View reviewed changes
@booxter
networking: Restore the original /etc/hosts on activation
b1db30c 
A patch that replaced the original file with a symlink to nix store was
reverted because MacOS Network framework doesn't support symlinks for
the file.

The revert leaves the system without any /etc/hosts file at all though.
To fix this, an activation step is added to restore the original file
from .before-nix-darwin backup, if it exists.

Signed-off-by: Ihar Hrachyshka <ihar.hrachyshka@gmail.com>
@booxter booxter dismissed stale reviews from Enzime and emilazy via b1db30c February 26, 2025 21:55
@booxter booxter marked this pull request as draft February 26, 2025 21:55
@booxter
Copy link
Contributor Author

booxter commented Feb 26, 2025

Hold off; I'm testing it. Will reopen for review when done.

@booxter
Copy link
Contributor Author

booxter commented Feb 26, 2025 •
edited
Loading

How I tested:

  1. Removed /etc/hosts. (Moved it to /etc/hosts.bak.)
  2. Ran darwin switch.
  3. Confirmed restoring /etc/hosts... message in the output.
  4. Confirmed that /etc/hosts is restored.
  5. Confirmed that /etc/hosts.before-nix-darwin no longer exists.
  6. Repeat step (2). No message anymore. Return code still zero.

@booxter booxter marked this pull request as ready for review February 26, 2025 22:01
@booxter booxter requested review from Enzime and emilazy February 26, 2025 22:02
@booxter
Copy link
Contributor Author

booxter commented Feb 26, 2025

@emilazy @Enzime this is ready to go. 🙏

emilazy
emilazy approved these changes Feb 26, 2025
View reviewed changes
Copy link
Collaborator

@emilazy emilazy left a comment

Choose a reason for hiding this comment

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

Seems like an appropriate quick fix for now.

@emilazy emilazy merged commit 31631ea into LnL7:master Feb 26, 2025
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@emilazy emilazy emilazy approved these changes

@Enzime Enzime Awaiting requested review from Enzime

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@booxter @emilazy @Enzime