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

zed-editor: add installRemoteServer option #6330

Merged
merged 1 commit into from
Jan 29, 2025

Conversation

GaetanLepage
Copy link
Member

@GaetanLepage GaetanLepage commented Jan 17, 2025

Description

Building on top of NixOS/nixpkgs#370017.
This option automatically symlinks the Zed remote server binary to the right location in the user's home directory.
It allows to connect to this host from a remote Zed instance.

See https://wiki.nixos.org/wiki/Zed#Remote_Server for more details.

cc @niklaskorz

Note: Requires NixOS/nixpkgs#370017 to land on the unstable channels.

Checklist

  • Change is backwards compatible.

  • Code formatted with ./format.

  • Code tested through nix-shell --pure tests -A run.all or nix develop --ignore-environment .#all using Flakes.

  • Test cases updated/added. See example.

  • Commit messages are formatted like

    {component}: {description}
    
    {long description}
    

    See CONTRIBUTING for more information and recent commit messages for examples.

  • If this PR adds a new module

    • Added myself as module maintainer. See example.

Maintainer CC

cc @libewa

@libewa
Copy link
Contributor

libewa commented Jan 17, 2025

Looks like the tests didn't run using the latest nixpkgs.

@GaetanLepage
Copy link
Member Author

Looks like the tests didn't run using the latest nixpkgs.

Indeed, the PR in question has not landed to nixos-unstable yet: https://nixpkgs-tracker.ocfox.me/?pr=370017

@GaetanLepage GaetanLepage force-pushed the zed-server branch 5 times, most recently from 28f8854 to d556b7a Compare January 24, 2025 08:14
@GaetanLepage
Copy link
Member Author

0.170.4 is already out... Those guys are a bit too fast !

Copy link

@niklaskorz niklaskorz left a comment

Choose a reason for hiding this comment

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

Tested and works fine for providing the remote server binary on a NixOS server

Copy link
Collaborator

@teto teto left a comment

Choose a reason for hiding this comment

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

could you test the executable is created at the correct location.
Does it need some special firewall rules ? maybe link towards them directly . Also I would disable it by default. Not everyone wants its machine to be accessible remotely.

I dont think we can setup them from HM alone and a correct approach is out of scope.
Thinking out loud: maybe HM modules could generate iptables scripts for the user to load.

@GaetanLepage
Copy link
Member Author

could you test the executable is created at the correct location.

I did so initially but after a fierce fight against the package-stub implementation of HM tests, I gave up.

Does it need some special firewall rules ?

I don't think so.

Also I would disable it by default. Not everyone wants its machine to be accessible remotely.

Fine by me. WDYT @niklaskorz ?

@niklaskorz
Copy link

niklaskorz commented Jan 28, 2025

Does it need some special firewall rules ?

I don't think so.

It only requires an SSH connection, I think it should be left to the user how that is established.

Also I would disable it by default. Not everyone wants its machine to be accessible remotely.

Fine by me. WDYT @niklaskorz ?

Sounds reasonable, although the binary by itself doesn't make the machine remotely accessible (OpenSSH does). But nonetheless, you don't need to download remote_server unless you want to use the machine as a remote server, so that avoids unnecessary downloads for most users.

@GaetanLepage GaetanLepage force-pushed the zed-server branch 2 times, most recently from 582ec5b to f597df9 Compare January 28, 2025 12:32
@GaetanLepage
Copy link
Member Author

Thanks @rycee !

@rycee rycee merged commit 1b4f2a4 into nix-community:master Jan 29, 2025
3 checks passed
@rycee
Copy link
Member

rycee commented Jan 29, 2025

Thanks for the contribution! I made some minor changes and added a basic test. Merged now 🙂

@GaetanLepage GaetanLepage deleted the zed-server branch January 29, 2025 11:04
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.

5 participants