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

fix: explicitly add systemctl and grep to path in shell-wrapper #240

Merged
merged 1 commit into from
Apr 11, 2023

Conversation

K900
Copy link
Contributor

@K900 K900 commented Apr 4, 2023

We can generally find them in /run/current-system/sw/bin, but WSL may try to call those before activation is fully done, so make sure it can always find what it needs.

We can generally find them in /run/current-system/sw/bin, but WSL
may try to call those before activation is fully done, so make sure
it can always find what it needs.
@@ -4,6 +4,7 @@ with lib;

let
bashWrapper = pkgs.writeShellScriptBin "sh" ''
export PATH=${lib.makeBinPath [ pkgs.systemd pkgs.gnugrep ]}
. ${config.system.build.etc}/etc/set-environment
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't this PATH get set here? Not sure where a command should be executed here. I am probably missing some context

Copy link
Contributor Author

Choose a reason for hiding this comment

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

/etc/set-environment sets PATH to /run/current-system/sw/bin, but the /run/current-system symlink might not exist when WSL first tries to systemctl | grep things.

Copy link
Member

Choose a reason for hiding this comment

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

hmmm, I see. We should comment that.

@@ -4,6 +4,7 @@ with lib;

let
bashWrapper = pkgs.writeShellScriptBin "sh" ''
export PATH=${lib.makeBinPath [ pkgs.systemd pkgs.gnugrep ]}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
export PATH=${lib.makeBinPath [ pkgs.systemd pkgs.gnugrep ]}
export PATH=${lib.makeBinPath with pkgs; [ systemd gnugrep ]}

@nzbr
Copy link
Member

nzbr commented Apr 4, 2023

If we're doing that, shouldn't we just add environment.systemPackages (and environment.defaultPackages) to path instead of some packages explicitly and sourcing /etc/set-environment?

@K900
Copy link
Contributor Author

K900 commented Apr 5, 2023

I'm not sure, I'd rather have the bare minimum needed by the WSL machinery here and handle the rest in a way that's as conventional as possible.

@nzbr
Copy link
Member

nzbr commented Apr 5, 2023

We could also just put a busybox in there. That would be future-proof as well and less easy to brick by changing the system config

@K900
Copy link
Contributor Author

K900 commented Apr 5, 2023

They do call systemctl, so we can't just put a busybox in there.

@nzbr
Copy link
Member

nzbr commented Apr 5, 2023

Right, I meant that more as "instead of gnugrep"

@K900
Copy link
Contributor Author

K900 commented Apr 10, 2023

I'm 100% expecting WSL to assume gnugrep there tbh.

@K900 K900 merged commit 3957b46 into nix-community:main Apr 11, 2023
@nzbr nzbr added the bug Something isn't working label Jul 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants