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

nixos/xonsh: source NixOS environment #84330

Merged
merged 1 commit into from May 1, 2020
Merged

Conversation

@das-g
Copy link
Member

das-g commented Apr 5, 2020

Without doing that, xonsh is unusable as login shell.

⚠️ Note that even with that change, setting users.users.<name?>.shell to xonsh for my user prevents me from successfully logging into GNOME 3 from GDM (and maybe prevents any graphical login - didn't test with other login managers or DEs / WMs, yet). But at least I get a usable xonsh shell in text mode (virtual terminal accessible with Ctrl-Alt-F1 etc.), which I didn't get without this change. (Without the change, xonsh would start on login, but would be nearly unusable as almost all external commands would have to be invoked through their absolute path, because they wouldn't be on $PATH.)

Motivation for this change

If a shell can be allowed as an interactive login shell (and NixOS option programs.xonsh.enable does that, if I understand correctly), it should be usable as a login shell, and logging into it should yield that shell in a usable state.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix run nixpkgs.nixpkgs-review -c nixpkgs-review pr 84330
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

Result of nixpkgs-review pr 84330 1
[empty]

nixos/modules/programs/xonsh.nix Outdated Show resolved Hide resolved
@das-g
Copy link
Member Author

das-g commented Apr 7, 2020

/cc current maintainers of the xonsh package: @spwhitt, @vrthra

@nixos-discourse
Copy link

nixos-discourse commented Apr 11, 2020

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

https://discourse.nixos.org/t/can-i-use-xonsh-as-my-login-shell-if-so-how/6472/8

@nixos-discourse
Copy link

nixos-discourse commented Apr 18, 2020

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

https://discourse.nixos.org/t/prs-ready-for-review-may-2019/3032/142

@das-g das-g force-pushed the das-g:xonsh-source-nixos-env branch from f581c64 to 57a56af Apr 18, 2020
@das-g das-g requested a review from rnhmjoj Apr 18, 2020
Copy link
Contributor

rnhmjoj left a comment

It looks good: I can log in, startx works, the environment seems ok.
There is only one issue I noticed: the warning about aliases (only ls for me) beign ignored because they would override a xonsh built-in alias. I'm not sure which option between ignoring with --suppress-skip-message or overwriting with --overwrite-alias is better but the warning is pretty annoying if you are using this as your daily shell.

Without doing that, xonsh is unusable as login shell
@das-g das-g force-pushed the das-g:xonsh-source-nixos-env branch from 57a56af to 347e251 Apr 21, 2020
@das-g
Copy link
Member Author

das-g commented Apr 21, 2020

There is only one issue I noticed: the warning about aliases (only ls for me) beign ignored because they would override a xonsh built-in alias. I'm not sure which option between ignoring with --suppress-skip-message or overwriting with --overwrite-alias is better but the warning is pretty annoying if you are using this as your daily shell.

Xonsh doesn't distinguish between aliases and its builtin functions:
>>> aliases
xonsh.aliases.Aliases(
{'EOF': <function xonsh.aliases.xonsh_exit>,
 'bg': <function xonsh.jobs.bg>,
 'cd': <function xonsh.dirstack.cd>,
 'completer': <function xonsh.completers._aliases.completer_alias>,
 'dirs': <function xonsh.dirstack.dirs>,
 'egrep': ['egrep', '--color=auto'],
 'exec': <function xonsh.aliases.xexec>,
 'exit': <function xonsh.aliases.xonsh_exit>,
 'fg': <function xonsh.jobs.fg>,
 'fgrep': ['fgrep', '--color=auto'],
 'grep': ['grep', '--color=auto'],
 'history': <function xonsh.history.main.history_main>,
 'ipynb': ['jupyter', 'notebook', '--no-browser'],
 'jobs': <function xonsh.jobs.jobs>,
 'popd': <function xonsh.dirstack.popd>,
 'pushd': <function xonsh.dirstack.pushd>,
 'quit': <function xonsh.aliases.xonsh_exit>,
 'replay': <function xonsh.replay.replay_main>,
 'scp-resume': ['rsync', '--partial', '-h', '--progress', '--rsh=ssh'],
 'showcmd': <function xonsh.aliases.showcmd>,
 'source': <function xonsh.aliases.source_alias>,
 'source-bash': ['source-foreign', 'bash', '--sourcer=source'],
 'source-cmd': <function xonsh.aliases.source_cmd>,
 'source-foreign': <function xonsh.aliases.source_foreign>,
 'source-zsh': ['source-foreign', 'zsh', '--sourcer=source'],
 'timeit': <function xonsh.timings.timeit_alias>,
 'trace': <function xonsh.aliases.trace>,
 'which': <function xonsh.xoreutils.which.which>,
 'xexec': <function xonsh.aliases.xexec>,
 'xonfig': <function xonsh.aliases.xonfig>,
 'xonsh-reset': <function xonsh.aliases.xonsh_reset>,
 'xontrib': <function xonsh.xontribs.xontribs_main>,
 'xpip': ['sudo',
  '/nix/store/ljbn79cwlwvrlmqil0nk4zvf043ph592-python3-3.7.5/bin/python3.7',
  '-m',
  'pip']})

Therefore, I'm reluctant to --overwrite-alias which potentially would let the NixOS environment silently override important xonsh commands.

On the other hand, --suppress-skip-message could silently discard aliases the user specified in NixOS option environment.shellAliases.

Thus I think we should gracefully handle the known alias collision(s), but have that warning message appear again for unexpected collisions. For an otherwise default configuration, ls seems indeed to be the only colliding alias name.

Xonsh sets ls to ls --color=auto -v while Bash picks up the ls alias ls --color=tty from a environment.shellAliases default override value. Those are similar enough that it shouldn't much matter, which one we choose. I arbitrarily decided to keep xonsh's and have implemented that in this change to this PR.

@das-g das-g requested a review from rnhmjoj Apr 21, 2020
Copy link
Contributor

rnhmjoj left a comment

It sounds like a good idea. The only problem is that if new aliases were to be addded to NixOS the list will have to be update manually, however environment.shellAliases doesn't change often so I guess it's all right.

@rnhmjoj
Copy link
Contributor

rnhmjoj commented May 1, 2020

If you have no objections I'm going to merge this: it's already an improvement over the current state of xonsh.

@das-g
Copy link
Member Author

das-g commented May 1, 2020

If you have no objections I'm going to merge this:

Me? Despite my warning in the PR description, I have no objections to merging this, or I would have filed it as WIP PR instead of regular PR.

The warning is just there to manage expectations. 😉

it's already an improvement over the current state of xonsh.

That's my take, too, @rnhmjoj. :shipit: Let's ship it!

Should it be backported to 20.03 or would that be too surprising a change?

@rnhmjoj
Copy link
Contributor

rnhmjoj commented May 1, 2020

That's my take, too, @rnhmjoj. :shipit: Let's ship it!

Good, let's do it.

Should it be backported to 20.03 or would that be too surprising a change?

Uhm, I'm not too keen on backporting non-bugfix changes after the feature-freeze. It should probably be safe but it may have some unintended consequences...

@rnhmjoj rnhmjoj merged commit 6c142fd into NixOS:master May 1, 2020
13 checks passed
13 checks passed
Evaluation Performance Report Evaluator Performance Report
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="347e251"; rev="347e2512613b08d68909226f2f23763e818857d0"; } ./pkgs/t
Details
grahamcofborg-eval-lib-tests nix-build --arg pkgs import ./. {} ./lib/tests/release.nix
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="347e251"; rev="347e2512613b08d68909226f2f23763e818857d0"; } ./nixos/
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="347e251"; rev="347e2512613b08d68909226f2f23763e818857d0"; } ./nixos/
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="347e251"; rev="347e2512613b08d68909226f2f23763e818857d0"; } ./nixos/
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="347e251"; rev="347e2512613b08d68909226f2f23763e818857d0"; } ./pkgs/t
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="347e251"; rev="347e2512613b08d68909226f2f23763e818857d0"; } ./pkgs/t
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="347e251"; rev="347e2512613b08d68909226f2f23763e818857d0"; } ./pkgs/t
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
@das-g das-g deleted the das-g:xonsh-source-nixos-env branch May 1, 2020
@das-g
Copy link
Member Author

das-g commented May 4, 2020

⚠️ Note that even with that change, setting users.users.<name?>.shell to xonsh for my user prevents me from successfully logging into GNOME 3 from GDM (and maybe prevents any graphical login - didn't test with other login managers or DEs / WMs, yet). But at least I get a usable xonsh shell in text mode (virtual terminal accessible with Ctrl-Alt-F1 etc.), which I didn't get without this change. (Without the change, xonsh would start on login, but would be nearly unusable as almost all external commands would have to be invoked through their absolute path, because they wouldn't be on $PATH.)

Tried it on nixos-unstable now and it works fine with GNOME 3. 🎉

Dunno what was up when I tried it on release-19.09

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.