Check if the selected shell is a directory

[HKalbasi]

Setting user.shell = pkgs.fish or similar is a common footgun that completely breaks the nix-on-droid. See #284.

This PR try to address the problem by checking that if the shell is a directory and falling back to bash in that case. This fix the common cause that many people hit that issue, but a general fix for that issue should probably include a way to switch generations without using the terminal, since setting the user.shell wrongly isn't probably the only way to break nix-on-droid.

[t184256]

is it possible to catch this at evaluation time?

[HKalbasi]

I added a check at evaulation time, but kept the old checks as a kind of defense in depth. I only tested it using nix eval so I'm not super sure that it will work.

[t184256]

OK, maybe I'm asking for the impossible, because pathExists will fail if it's not instantiated yet =/

[HKalbasi]

Ah, that's sad. I searched a bit and didn't found a way, so I reverted to the initial state. But my nix-fu is not great so if you have an idea about implementing the check at the evaluation time, I will implement it.

[t184256]

Just to be clear, ain't your fault that you didn't deliver when I asked for impossible.

[t184256]

Unlike NixOS

Got me curious about what magic is NixOS doing then, and it looks like it accepts derivations as long as they set passthru.shellPath = ..., and then it's smart enough to append that.

Would it make sense to follow suit and:

• accept arbitrary strings (and use your current code to catch those who cast dirs to strings with a user.shell = "${pkgs.fish}")
• reject user.shell that has .type == "derivation", except
• it has a .passthru.shellPath, in which case accept and append it?

[HKalbasi]

That is great. I implemented it but tested it only on nix eval, so a double check would be great.

[t184256]

Yeah, looks great! Don't worry, I'll (eventually) cover this with tests, likely by extending poke_around.py. Note to self, test:

1. pkgs.fish -> switching succeeds, fish after re-login
2. ${pkgs.fish} -> eval aborts with directory warning
3. ${pkgs.fish}/bin/fish -> switching succeeds, fish after re-login

[t184256]

Hi, I've been writing tests for the feature you've implemented, and they've uncovered a small problem (should be visible if you go to CI, emulate (29, poke_around) job, Upload screenshots phase, download and inspect the last screenshots. Could you, please:

1. fix the swallowing of user.shell in the output?
2. review my tests?

[HKalbasi]

I fixed the problem, and also removed the "unlike nixos" part since now we are similar to the nixos if I understand correctly.

Tests look good to me, I just wonder why you didn't use f-strings instead of .replace('%SHELL%', shell)?

By the way, I really enjoyed your test infrastructure.

[t184256]

I fixed the problem, and also removed the "unlike nixos" part since now we are similar to the nixos if I understand correctly.

Yeah, makes sense.

Tests look good to me, I just wonder why you didn't use f-strings instead of .replace('%SHELL%', shell)?

I found it more readable than duplicating all the curly brackets.

By the way, I really enjoyed your test infrastructure.

Thank you. =)

[HKalbasi]

I guess this time the failure is random?

[t184256]

Yep. Everything looks in order now. I'd appreciate if you integrated the fixes into the 1st and 2nd commits respectively, so that both resulting commits would be "green", but if you don't want to, I can do it later myself. Thank you!