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

Is it possible to print to standard error in config.fish? #2702

Closed
da99 opened this issue Jan 31, 2016 · 10 comments
Closed

Is it possible to print to standard error in config.fish? #2702

da99 opened this issue Jan 31, 2016 · 10 comments
Assignees
Labels
Milestone

Comments

@da99
Copy link

@da99 da99 commented Jan 31, 2016

The last one is the only one that gets printed to the console in config.fish:

 echo "started 1" >&2
 echo "started 2" 1>&2
 echo "started 3"           # this one gets printed

Is this normal?

@oranja
Copy link

@oranja oranja commented Jan 31, 2016

This is the current design, yes (source).
Personally I prefer to see errors if they exist, but I assume they are redirected in this case to avoid a clutter of warnings and errors on Fish initializations.

@ridiculousfish
Copy link
Member

@ridiculousfish ridiculousfish commented Jan 31, 2016

When fish sources config.fish, it redirects stderr to /dev/null. It's been this way since literally the very first commit in 2005. My guess is that it's to handle the case where the file doesn't exist.

@ridiculousfish ridiculousfish added this to the fish-tank milestone Jan 31, 2016
@da99 da99 closed this Jan 31, 2016
@oranja
Copy link

@oranja oranja commented Jan 31, 2016

My guess is that it's to handle the case where the file doesn't exist.

I think most can agree that it's a poor choice to leave it like this if that is true and it's still the only reason.
Others might disagree and suggest that people who make changes to config.fish should run their tests before adding to the config. I'd counter, ahead of time, by arguing that sometimes things break after a while from changes that seem unrelated.

@krader1961
Copy link
Contributor

@krader1961 krader1961 commented Jan 31, 2016

That behavior is so wrong I am amazed no one has changed it. If a command in my config.fish doesn't exist or writes to stderr I want to know it. I don't want the error message to be silently discarded. If scripts that are part of the fish distribution which might be invoked from config.fish (e.g., fish_vi_mode) write messages to stderr that should be ignored then the offending commands in those scripts should redirect stderr. But to blindly discard all stderr text is a really bad idea. The only possible exception might be for non-interactive invocations but even that is probably a bad idea.

It's trivial to test whether config.fish exists and is readable and skip sourcing it if it is not.

@faho
Copy link
Member

@faho faho commented Feb 1, 2016

I have to agree with @krader1961. Errors from config.fish should not be ignored - there shouldn't be a message if it doesn't exist (fish is still usable without it), but if there's anything wrong I'd want to be notified.

Hence, I'm reopening this.

@faho faho reopened this Feb 1, 2016
@krader1961
Copy link
Contributor

@krader1961 krader1961 commented Feb 1, 2016

BTW, I've already written a patch that fixes this. I haven't created a pull-request yet because I'm trying to figure out if sourcing /usr/local/share/fish/config.fish and /usr/local/etc/fish/config.fish during unit tests is correct (that's in addition to the test specific config.fish that is sourced last). It seems to me it is not correct as the unit tests are supposed to be hermetic; i.e., not influenced by the locally installed fish.

@krader1961
Copy link
Contributor

@krader1961 krader1961 commented Feb 3, 2016

Making the unit tests more hermetic is possible. The change is pretty straightforward thanks to fish's "relocatable" feature that causes it to look for its support files beneath the parent directory where the fish binary is located. But it does require a lot of individual changes due to all the ../fish references in the tests. It's also not necessary for the core change that fixes this issue. So I'm going to open a separate issue for fixing the non-hermetic aspect of the tests.

@krader1961
Copy link
Contributor

@krader1961 krader1961 commented Feb 4, 2016

My fix for this exposed a mistake in my config.fish which contained this statement:

test $SSH_AUTH_SOCK = '' -a -f ~/.ssh/auth_sock

Note the lack of quotes around the variable. Since the var wasn't set in the login shell it was resulting in the error test: Unexpected argument type at index 0. And that error was being swallowed by the stderr redirection. Yay!

ridiculousfish added a commit that referenced this issue Feb 4, 2016
All versions of fish prior to this change silently discarded anything written
to stderr while source a config.fish file. Apparently just to avoid having
the source command display an error if the file did not exist. This can mask
real problems. So instead this change explicitly checks whether the file is
readable and silently skips sourcing it if it isn't.

Resolves issue #2702.
@krader1961
Copy link
Contributor

@krader1961 krader1961 commented Feb 7, 2016

This can be closed. My PR #2708 fixes this issue.

@zanchey zanchey modified the milestones: next-2.x, fish-tank Feb 7, 2016
@zanchey
Copy link
Member

@zanchey zanchey commented Feb 7, 2016

Grand!

@zanchey zanchey closed this Feb 7, 2016
zanchey added a commit that referenced this issue Feb 22, 2016
I noticed while fixing issue #2702 that the fish program being tested
was sourcing config.fish files outside of the current build. This also
happens when Travis CI runs the tests but isn't an issue there because
of how Travis is configured to execute the tests.

I also noticed that running `make test` was polluting my personal fish
history; which will become a bigger problem if and when the fishd universal
var file is moved from $XDG_CONFIG_HOME to $XDG_DATA_HOME.

This change makes it possible for an individual to run the tests on
their local machine secure in the knowledge that only the config.fish and
related files from their git repository will be used and doing so won't
pollute their personal fish history.

Resolves #469
zanchey added a commit to zanchey/fish-shell that referenced this issue Feb 24, 2016
I noticed while fixing issue fish-shell#2702 that the fish program being tested
was sourcing config.fish files outside of the current build. This also
happens when Travis CI runs the tests but isn't an issue there because
of how Travis is configured to execute the tests.

I also noticed that running `make test` was polluting my personal fish
history; which will become a bigger problem if and when the fishd universal
var file is moved from $XDG_CONFIG_HOME to $XDG_DATA_HOME.

This change makes it possible for an individual to run the tests on
their local machine secure in the knowledge that only the config.fish and
related files from their git repository will be used and doing so won't
pollute their personal fish history.

Resolves fish-shell#469
krader1961 added a commit that referenced this issue Feb 26, 2016
Kurtis Rader
I noticed while fixing issue #2702 that the fish program being tested
was sourcing config.fish files outside of the current build. This also
happens when Travis CI runs the tests but isn't an issue there because
of how Travis is configured to execute the tests.

I also noticed that running `make test` was polluting my personal fish
history; which will become a bigger problem if and when the fishd universal
var file is moved from $XDG_CONFIG_HOME to $XDG_DATA_HOME.

This change makes it possible for an individual to run the tests on
their local machine secure in the knowledge that only the config.fish and
related files from their git repository will be used and doing so won't
pollute their personal fish history.

Resolves #469
@krader1961 krader1961 self-assigned this Mar 25, 2017
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants
You can’t perform that action at this time.