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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Port the flags of nix-daemon to nix daemon #8788

Merged
merged 9 commits into from Aug 28, 2023

Conversation

bryanhonof
Copy link
Member

@bryanhonof bryanhonof commented Aug 4, 2023

Motivation

The new nix daemon command doesn't accept the same flags that nix-daemon does.

Context

Ref #8785
Fixes #7128

Checklist for maintainers

Maintainers: tick if completed or explain if not relevant

  • agreed on idea
  • agreed on implementation strategy
  • tests, as appropriate
    • functional tests - tests/**.sh
    • unit tests - src/*/tests
    • integration tests - tests/nixos/*
  • documentation in the manual
  • documentation in the internal API docs
  • code and comments are self-explanatory
  • commit message explains why the change was made
  • new feature or incompatible change: updated release notes

Priorities

Add 馃憤 to pull requests you find important.

@github-actions github-actions bot added the new-cli Relating to the "nix" command label Aug 4, 2023
The new `nix daemon` command didn't accept the same flags that `nix-daemon` did.
@bryanhonof bryanhonof force-pushed the bryanhonof/nix-daemon-flag-port branch from b3649b6 to 2c94ac5 Compare August 4, 2023 22:57
src/nix/daemon.cc Outdated Show resolved Hide resolved
Co-authored-by: Eelco Dolstra <edolstra@gmail.com>
Copy link
Member

@Ericson2314 Ericson2314 left a comment

Choose a reason for hiding this comment

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

We can declaratively mark flags as experimental with the new CLI

src/nix/daemon.cc Outdated Show resolved Hide resolved
src/nix/daemon.cc Outdated Show resolved Hide resolved
src/nix/daemon.cc Outdated Show resolved Hide resolved
bryanhonof and others added 3 commits August 8, 2023 10:09
Co-authored-by: John Ericson <git@JohnEricson.me>
Co-authored-by: John Ericson <git@JohnEricson.me>
Co-authored-by: John Ericson <git@JohnEricson.me>
@bryanhonof
Copy link
Member Author

bryanhonof commented Aug 8, 2023

We can declaratively mark flags as experimental with the new CLI

@Ericson2314 I'm getting the following warning, which, I believe, is just fixed by switching .handler and .experimentalFeature.

src/nix/daemon.cc:518:13: warning: ISO C++ requires field designators to be specified in declaration order; field 'experimentalFeature' will be initialized after field 'handler' [-Wreorder-init-list]
            .handler = {[&]() {
            ^~~~~~~~~~~~~~~~~~~
src/nix/daemon.cc:517:36: note: previous initialization for field 'experimentalFeature' is here
            .experimentalFeature = Xp::DaemonTrustOverride,
                                   ^~~~~~~~~~~~~~~~~~~~~~~

However, what I find more concerning, when I now run the nix daemon command with --force-untrusted it just seems to work without enabling the experimental feature. Whilst before, it did error out.

Before:

$ ./outputs/out/bin/nix --extra-experimental-features nix-command daemon --force-untrusted --stdio
error: experimental Nix feature 'daemon-trust-override' is disabled; use '--extra-experimental-features daemon-trust-override' to override

After:

$ # Simply waits for input it seems like
$ ./outputs/out/bin/nix --extra-experimental-features nix-command daemon --force-untrusted --stdio


addFlag({
.longName = "force-trusted",
.description = "Forces the daemon to trust connecting clients, forwarding the connection without the receiving daemon processing it.",
Copy link
Member

Choose a reason for hiding this comment

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

Does this actually forward the connection? I assume that depends on nix daemon's own --store parameter: If it's "local", then it won't forward to another daemon.

Copy link
Member

Choose a reason for hiding this comment

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

That is true. Forcing untrusted does make not force in the --stdio case when --store is a RemoteStore subclass, but forcing trusted (if I am reading the code above right) doesn't seem to affect when forwarding happens.

Copy link
Member Author

Choose a reason for hiding this comment

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

So, just "Forces the daemon to trust connecting clients." would be enough as a description in this case?

Copy link
Member

Choose a reason for hiding this comment

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

Yes I think that is good

@tomberek tomberek mentioned this pull request Aug 25, 2023
86 tasks
src/nix/daemon.cc Outdated Show resolved Hide resolved

addFlag({
.longName = "force-untrusted",
.description = "Forces the daemon to not trust connecting clients, the connection will be processed by the receiving daemon before forwarding commands.",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
.description = "Forces the daemon to not trust connecting clients, the connection will be processed by the receiving daemon before forwarding commands.",
.description = "Force the daemon to not trust connecting clients. The connection will be processed by the receiving daemon before forwarding commands.",

@fricklerhandwerk
Copy link
Contributor

fricklerhandwerk commented Aug 28, 2023

Reviewed in Nix team meeting for stabilisation effort:

  • agreed on the change
  • the new command is not tested, but runs the exact same code as the old one
    • @bryanhonof if you have time, could you contribute something towards testing the daemon at the component level rather through the CLI?
  • fixed up documentation
  • merging

src/nix/daemon.cc Outdated Show resolved Hide resolved
@tomberek tomberek enabled auto-merge (squash) August 28, 2023 12:46
@bryanhonof
Copy link
Member Author

  • @bryanhonof if you have time, could you contribute something towards testing the daemon at the component level rather through the CLI?

@fricklerhandwerk Wouldn't mind giving it a shot. Where can I find examples/docs of this?

@tomberek tomberek merged commit 736b9ce into NixOS:master Aug 28, 2023
8 checks passed
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/2023-08-28-nix-team-meeting-minutes-83/32290/1

@bryanhonof bryanhonof deleted the bryanhonof/nix-daemon-flag-port branch August 31, 2023 20:21
@thufschmitt thufschmitt added this to the CLI Stabilisation milestone Dec 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation new-cli Relating to the "nix" command
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

nix daemon should accept --stdio flag
7 participants