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

Use proper option types for wsl.conf #153

Merged
merged 3 commits into from
Nov 6, 2022
Merged

Conversation

nzbr
Copy link
Member

@nzbr nzbr commented Nov 2, 2022

Added proper types for all options that can be set in wsl.conf to avoid typos and using wrong types

@nzbr nzbr added the enhancement New feature or request label Nov 2, 2022
@nzbr nzbr requested a review from SuperSandro2000 November 2, 2022 22:54
modules/wsl-conf.nix Outdated Show resolved Hide resolved
description = "Support running Windows binaries from the linux shell.";
};
appendWindowsPath = mkOption {
type = bool;
Copy link
Member

Choose a reason for hiding this comment

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

This is currently a string which contains the boolean.

error: A definition for option `wsl.wslConf.interop.appendWindowsPath' is not of type `boolean'. Definition values:
       - In `/nix/store/byrkin8wmr9hbzqhnsfa01bj2g7xij81-source/common/modules.nix': "false"

Copy link
Member Author

Choose a reason for hiding this comment

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

Where is that string? Was it an old default config? The current default configuration.nix as well as my flake config build just fine

Copy link
Member

Choose a reason for hiding this comment

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

It is some option I have currently set but I no longer know why and from where I got it. Also can't find any reference in code.

Copy link
Member

Choose a reason for hiding this comment

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

I think I now know the root cause of this: before wslConf was a freeform setting that accepted a lot more than it should. Now that the options are properly typed errors are properly surfaced and you can no longer set an identical string to an otherwise boolean option.

systemd = mkOption {
type = bool;
default = false;
description = "Use systemd as init. There's no need to enable this manually, use the wsl.nativeSystemd option instead";
Copy link
Member

Choose a reason for hiding this comment

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

We should assert or warn this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea, I'll add a warning

boot = {
command = mkOption {
type = str;
default = "";
Copy link
Member

Choose a reason for hiding this comment

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

example would be great.

Copy link
Member Author

Choose a reason for hiding this comment

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

This isn't really useful on NixOS, because we have systemd. The command is started using the root shell, so syschdemd would drop it to user privileges anyway. MS intends this to be used to start a daemon in the background (their example is starting docker)

@nzbr nzbr requested a review from SuperSandro2000 November 3, 2022 18:11
Copy link
Member

@SuperSandro2000 SuperSandro2000 left a comment

Choose a reason for hiding this comment

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

didn't test again but looks alright

@nzbr nzbr merged commit 318fc78 into nix-community:main Nov 6, 2022
@nzbr nzbr mentioned this pull request Nov 6, 2022
psvo added a commit to psvo/NixOS-WSL that referenced this pull request Nov 21, 2022
* upstream/main:
  Fix syschdemd exit code (nix-community#140)
  update remaining references to wsl.automountPath (nix-community#158)
  Update flakes (nix-community#124)
  wsl.conf: proper option types (nix-community#153)
  Fix eval by disabling module completely when wsl.enable = false (nix-community#151)
  NixOS users.users.<user> attribute name and .name can differ (nix-community#147)
  switch nixpkgs.overlays to inline overlay (nix-community#150)
  feat: native systemd support (nix-community#134)
  remove boot.isContainer (nix-community#145)
  Do not import minimal profile by default (nix-community#144)
@nzbr nzbr changed the title wsl.conf: proper option types Use proper option types for wsl.conf Jul 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants