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

Unintended loading xonshrc file in script mode #5488

Closed
anki-code opened this issue Jun 11, 2024 · 15 comments · Fixed by #5491
Closed

Unintended loading xonshrc file in script mode #5488

anki-code opened this issue Jun 11, 2024 · 15 comments · Fixed by #5491

Comments

@anki-code
Copy link
Member

anki-code commented Jun 11, 2024

My xonshrc is pretty silent and I suddenly noticed that xonshrc is loaded in script mode:

cd /tmp
echo 'echo RC' >> ~/.xonshrc
echo '#!/usr/bin/env xonsh' > myscript
chmod +x myscript

./myscript
# RC

xonsh myscript.xsh
# RC

When I run any xonsh script I have no any expectation that my huge .xonshrc with features for prompt will be loaded.

Bash/zsh also don't load RC files when you run script:

cd /tmp
echo 'echo RC' >> ~/.bashrc
echo 'echo 1' > script.sh
bash script.sh
# 1  # No RC

echo 'echo RC' >> ~/.zshrc
echo 'echo 1' > script.zsh
zsh script.zsh
# 1  # No RC

For community

⬇️ Please click the 👍 reaction instead of leaving a +1 or 👍 comment

@anki-code
Copy link
Member Author

anki-code commented Jun 11, 2024

I think this is the result of #5099 (@cosineFish @bestlem).

@gforsyth I think we need to fix/revert this in 0.17.0 because:

  • All xonsh scripts are affected.
  • If you have many libs in xonshrc and want to run xonsh script in env you will face with unintended errors about missing libs. It's bad experience.

The user own xonshrc that is mostly for using it in prompt must NOT be loaded in script mode.

@anki-code anki-code changed the title Unintended loading RC file in script mode Unintended loading ~/.xonshrc file in script mode Jun 11, 2024
@gforsyth
Copy link
Collaborator

The user own xonshrc that is mostly for using it in prompt must NOT be loaded in script mode.

I mean, that's the bash approach, but zsh and fish do load those files in script mode.

A user can run using --no-rc to avoid loading their xonshrc file

@anki-code
Copy link
Member Author

anki-code commented Jun 11, 2024

@gforsyth

  1. zsh does not do this (see example in the first message).
  2. I believe that if I have CLI xonsh script.xsh with shebang I should not do anything with --no-rc. I expect that it just works. See also xonsh-awesome-cli-app.

So I'm completely for disabling load ~/.xonshrc for scripts accordingly with expectations and bash, zsh behavior.
If user wants to create xonshrc to load forever on user base level we already have ~/.config/xonsh/rc.xsh (doc).

@gforsyth
Copy link
Collaborator

ah, right, zsh does load zshenv though -- I don't want to get into having a million special cased startup files, though.

I'm fine with this either way. If we do disable loading rc files in script mode, then we should ensure that a user can force loading them by passing -i

@anki-code
Copy link
Member Author

anki-code commented Jun 11, 2024

I understand the idea from here to have this in every xonshrc file:

if $XONSH_INTERACTIVE:
    # ...

It looks clear BUT you forget about software around (e.g. conda, zoxide, xonsh wizard, xonsh web, etc) that just do:

echo "some_init_code()" >> ~/.xonshrc

And this kind of tools don't know about $XONSH_INTERACTIVE. And when they add init code into xonshrc directly ALL scripts will be affected by this and will produce errors and slow timings! This is why I think it's a not so good idea @gforsyth .

We need more complex behavior. I'm thinking about at least push out ~/.xonshrc to interactive mode.

@anki-code
Copy link
Member Author

anki-code commented Jun 11, 2024

And one moment. I want to remember you all that we have already stepped on this rake when we had loading history on every xonsh run - #4178. Before this fix we observed all kinds of unexpected errors and bad experience with scripts.
Do not do unexpected unintended work undercover.

@bestlem
Copy link
Contributor

bestlem commented Jun 11, 2024

I understand the idea from here to have this in every xonshrc file:

if $XONSH_INTERACTIVE:
    # ...

It looks clear BUT you forget about software around (e.g. conda, zoxide, xonsh wizard, xonsh web, etc) that just do:

echo "some_init_code()" >> ~/.xonshrc

And this kind of tools don't know about $XONSH_INTERACTIVE. And when they add init code into xonshrc directly ALL scripts will be affected by this and will produce errors and slow timings! This is why I think it's a not so good idea @gforsyth .

We need more complex behavior. I'm thinking about at least push out ~/.xonshrc to interactive mode.

This is exactly how fish does it - load the same config.fish in all cases but have test for --is-interactive and --is-login Why is this not an issue for fish?

@anki-code
Copy link
Member Author

anki-code commented Jun 11, 2024

@bestlem I think it's issue for fish. Let's consider the situation. I want to write script myscript using fish and then I want to apply it for 1000 files. Why I need to load fish RC file with setting up fancy prompt 1000 times?
If the answer - "you must know about checking for interactive usage and add checking in the RC file or add --no-rc in your script" - I treat this as "you must know everything in the world" and I don't like this.

Second thing is from practice. All home ~/.*rc files xonsh/bash/zsh are ordinarily unmanageable trash on most of systems I saw and applying the code from there to every execution of scripts is the shoot in the leg.

I'm ok to keep the rest of RC places like it works now but not for ~/.xonshrc. I'm in preparing the PR.

@bestlem
Copy link
Contributor

bestlem commented Jun 11, 2024

Looking at my fish config item and fzf look at --is-interactive others don't affect script usage.
In my experience I use my ~/.zshenv, config.fish etc to enforce a sane set of constant stuff across all my environments. and the PATH and environment variables will differ between Windows / WSL / MacOS/ Solaris /NeXT / Linux and if my machine is in London or Chicago when travelling or remote logging in. I don't what to have to make each script I write deal with all those cases when I can with any other shell just change one or two files for each OS.

The environment configuration is not the job of my script.

Actually I have been commenting on SO question that you must understand what each line in your shell config files does. If you don't then leave that line out.
With experience you don't have to know the details of what that line does but you do have to understand its effect on your environment.

There MUST be a way to run xonsh for scripts loading ~/.xonshrc - if you don't want it then there is --no-rc which you can put in the shebang of your script.

The major problem before was that there was --no-rc but setting XONSH_INTERACTIVE was being done in the wrong place so I think if you loaded a config file ir set it to true in all cases.

@anki-code
Copy link
Member Author

anki-code commented Jun 11, 2024

@bestlem I'm thinking about ~/.xonshrc as an unmanageable buffer for random configurations. If you are experienced user who knows life you will use ~/.config/ and I assume you know what you are doing. So this is why in the PR I will make ~/.xonshrc active only of interactive mode to prevent unexpected and unintended using in scripts. The rest behavior will stay as implemented.

(JFYI I'm completely against of adding --no-rc to shebangs by default. This decision completely annihilates the value of /etc/xonsh/xonshrc and others. At the end we will have tons of tools with --no-rc in shebangs. What a hell will be.)

@bestlem
Copy link
Contributor

bestlem commented Jun 12, 2024

There are some inconsistencies here.

But the inconsistencies are that in all cases all the other shells read either both user and files in /etc or neither. ie the behaviour is never not read ~/.xonshrc but read /etc/xonsrc/*
Add to that you are suggesting that ~/.xonshrc and ~/.config/xonsrc are treated differently - that is just different to anything else and will mean the script depends on where a file is which is unusual.

I note that sh and bash behave differently to zsh and fish in that the former does not read anything but the latter do read some files for scripts.

I also work on Macs and Next which don't have a login shell. On other Unixes the login shell will already have been read, So I am used to having to get $PATH set by something and I don't want that in the script but something has to be read in e.g. using ~/.xonshrc etc,

@anki-code
Copy link
Member Author

anki-code commented Jun 12, 2024

First of all I described the perspective of view in the PR - #5491. Please also read the changes in docs/ section.

Why make xonsh behave differently to all other shells?

Bash and Zsh do not use ~/.bashrc and ~/.zshrc in non-interactive mode. Please don't say "all".
(And it's meaningful because every tool want to do echo "mycode" >> ~/.bashrc and you don't want to have overloaded execution of every bash script.)

I also work on Macs and Next which don't have a login shell. On other Unixes the login shell will already have been read, So I am used to having to get $PATH set by something and I don't want that in the script but something has to be read in e.g. using ~/.xonshrc etc,

👉 I think this is the key. Instead of describe your problem you try to change the xonsh behavior. Please describe how system where you want to use xonsh is working and what is the problem first.

@bestlem

This comment was marked as off-topic.

1 similar comment
@bestlem

This comment was marked as off-topic.

@anki-code
Copy link
Member Author

anki-code commented Jun 13, 2024

I see this in https://carapace-sh.github.io/carapace-bin/setup.html:

image

@bestlem are you happy that this code will be executed on every run of every script?
We have 100+ xontribs. Are they need to implement checking on interactive mode because they can be loaded in RC?

Personally Im not happy, but ok, let's live with this until facing with new information.

Maybe we can mitigate this by checking interactive mode in completer, history, prompt, xontrib-template, etc and do not load completers - #5495. It will be cool to work on this and report.

Maybe we need to have different path to init completions like in fish. But I don't like this path. If we want to keep interactive mode in mind we need to leave current behavior and checking it to make habit.

@anki-code anki-code changed the title Unintended loading ~/.xonshrc file in script mode Unintended loading xonshrc file in script mode Jun 13, 2024
gforsyth pushed a commit that referenced this issue Jun 13, 2024
### Motivation

In #5099 was introduced the logic where all RC files are loaded in
interactive and non-interactive modes. This logic is not working good
for home based `~/.xonshrc` file.

First of all `~/.xonshrc` is the buffer/accumulator of settings focused
on interactive mode. Most tools with integration with xonsh (e.g.
`conda`, `zoxide`, etc) offer to just do `echo "init_tool()" >>
~/.xonshrc` (`conda` has around 20 lines of init code) to start using
the tool and users are doing this without any doubts.

But because of after 5099 `~/.xonshrc` is executed in non-interactive
mode the adding code to it leads to unexpected and unintended side
effects:

* If you run a script with shebang (e.g. `#!/usr/bin/env xonsh` or
[xonsh-awesome-cli-app](https://github.com/anki-code/xonsh-awesome-cli-app))
or just from `xonsh script.xsh` the code will be unexpected and
unintended slower.

* If you're using xonsh-based tools (e.g. you install them using pip)
and run them in environment that has no packages that initiated in
`~/.xonshrc` you will see epic errors.

* Additional context: 
* Bash and Zsh do not load `~/.bashrc` and `~/.zshrc` in non-interactive
mode by the same reasons.
* We have welcome message `Create ~/.xonshrc file manually or use xonfig
to suppress the welcome message` and we don't want to make the process
of creating this file complex.

All of this leads to bad unexpected and unintended experience. This PR
is to fix this.

### Expectation

By doing this fix we assume that experienced user who wants to build
good repeatable run control files will use another ways to create config
files and this has [reflection in
docs](https://github.com/xonsh/xonsh/blob/8860f2bd5273d5f3fc08ccf6be6af8163bfec0bd/docs/xonshrc.rst).
In the nutshell if you want to create the RC files that affect every run
of code you should use one or many of these ways:

* Cross-desktop group (XDG) compliant `~/.config/xonsh/rc.xsh` control
file.
* The system-wide control file `/etc/xonsh/xonshrc` for Linux and OSX
and in `%ALLUSERSPROFILE%\xonsh\xonshrc` on Windows. It controls options
that are applied to all users of Xonsh on a given system.
* The home-based directory `~/.config/xonsh/rc.d/` and system
`/etc/xonsh/rc.d/` can contain .xsh files. They will be executed at
startup in order. This allows for drop-in configuration where your
configuration can be split across scripts and common and local
configurations more easily separated.

In your configs you need to check `$XONSH_INTERACTIVE` and
`$XONSH_LOGIN` explicitly.

### Before

`~/.xonshrc` is used in non-interactive mode.

```xsh
echo "echo RC" >> ~/.xonshrc
cd /tmp
echo "echo Script" > script.xsh
xonsh script.xsh
# RC
# Script
```
```xsh
cd /tmp
echo 'echo RC' >> ~/.xonshrc
echo '#!/usr/bin/env xonsh' > myscript
chmod +x myscript

./myscript
# RC
```

### After

`~/.xonshrc` is not used in non-interactive mode. Use `-i` if you need
it.

```xsh
echo "echo RC" >> ~/.xonshrc
cd /tmp
echo "echo Script" > script.xsh
xonsh script.xsh
# Script

xonsh -i script.xsh
# RC
# Script
```
```xsh
cd /tmp
echo 'echo RC' >> ~/.xonshrc
echo '#!/usr/bin/env xonsh' > myscript
chmod +x myscript

./myscript
```
Closes #5488 #4096 #5496

### Fun

I want to leave here some nice representation of how it works in
bash/sh/zsh from [twitter
post](https://twitter.com/paxx39/status/1742768007154479109):



![image](https://github.com/xonsh/xonsh/assets/1708680/cd7b3803-483f-4d5d-bf9d-baa61c794f68)


## For community
⬇️ **Please click the 👍 reaction instead of leaving a `+1` or 👍
comment**

---------

Co-authored-by: a <1@1.1>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants