Skip to content

Fish 2.6.0 broke setenv? #4103

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

Closed
x4121 opened this issue Jun 5, 2017 · 27 comments
Closed

Fish 2.6.0 broke setenv? #4103

x4121 opened this issue Jun 5, 2017 · 27 comments
Labels

Comments

@x4121
Copy link

x4121 commented Jun 5, 2017

Since 2.6.0 release rbenv stopped working and echos setenv: Too many arguments during init. This is caused from the first line in the init script setenv PATH '/home/adg/.rbenv/shims' $PATH and setenv not working with multiple values anymore. Is this intended behavior? The release notes state that setenv was changed, but this doesn't seem to be correct 😕

fish 2.6.0
Ubuntu 17.04
Tested on clean Ubuntu docker container with just fish and rbenv installed. config.fish just containing status --is-interactive; and . (rbenv init -|psub)

@krader1961
Copy link
Contributor

Can you show us the output of rbenv init -? The setenv command takes just one or two values. The first is the var name the second the value. You'll only get that error if it is being invoked with three or more values.

It was broken briefly (about 3 weeks) by the fix for issue #3897. The fix for that regression can be found in issue #3937 and is definitely in fish 2.6.0. Are you sure you're running 2.6.0? What does fish --version and echo $version report?

@krader1961 krader1961 added needs more info bug Something that's not working as intended labels Jun 5, 2017
@mistydemeo
Copy link
Contributor

I'm seeing this as well, with fish 2.6.0 and rbenv 1.1.0. The output of rbenv init - is:

setenv PATH '/Users/mistydemeo/.rbenv/shims' $PATH
setenv RBENV_SHELL fish
source '/usr/local/Cellar/rbenv/1.1.0/libexec/../completions/rbenv.fish'
command rbenv rehash 2>/dev/null
function rbenv
  set command $argv[1]
  set -e argv[1]

  switch "$command"
  case rehash shell
    source (rbenv "sh-$command" $argv|psub)
  case '*'
    command rbenv "$command" $argv
  end
end

I'm guessing the first line is the culprit.

@mistydemeo
Copy link
Contributor

It looks like this was fixed upstream in rbenv: rbenv/rbenv@be2e606 and rbenv/rbenv@a81da8d

I appreciate that rbenv was probably using setenv inappropriately to begin with, since it's there for csh compatibility and not a native function. Since the docs previously identified the function as a set -gx wrapper though, and since the changelog doesn't mention the new restrictions, the changes might be overly strict? Maybe it should nag about multiple arguments rather than quitting for a few release, along with a more direct changelog note to make the change more discoverable.

@krader1961
Copy link
Contributor

Then rbenv is broken. Start csh (from which setenv originates) and do this experiment:

$ setenv x a
$ setenv x a b
setenv: Too many arguments.

It's possible that the old fish implementation of setenv was broken and allowed that invalid set of arguments. But even if true rbenv should not have been relying on it.

@mistydemeo
Copy link
Contributor

It's possible that the old fish implementation of setenv was broken and allowed that invalid set of arguments.

It was, yeah. I think it's true rbenv shouldn't have relied on it, though the function's documentation explicitly described it as a set -xg wrapper:

function setenv --description 'Set global variable. Alias for set -g, made for csh compatibility'

@krader1961
Copy link
Contributor

krader1961 commented Jun 6, 2017

Sorry, but it is completely irrelevant that our docs said it was "a set -gx wrapper". That is both an implementation detail and a hint that you should use the fish native set command. It's unfortunate the old implementation allowed invalid syntax. And had someone noticed when fish 2.6.0 was in development or beta testing we would have entertained continuing to allow the less strict behavior. But that ship has sailed. It's far too late to reintroduce it.

@krader1961 krader1961 added question and removed bug Something that's not working as intended needs more info labels Jun 6, 2017
@mistydemeo
Copy link
Contributor

Can the change at least be documented? Right now the documentation only says this:

export and setenv now understand colon-separated PATH, CDPATH and MANPATH variables.

It doesn't mention that setenv was reimplemented and that it's now more restrictive (as it was probably intended to be).

@krader1961
Copy link
Contributor

Right now the documentation only says this...

Well, strictly speaking the CHANGELOG only documents what was changed and the current statement is correct. We don't actually document the setenv compatibility wrapper. And the change only inadvertently made the command behave properly. I didn't deliberately set out to change that behavior and the fact it did change would be invisible unless you're relying on undocumented behavior 😄

Having said all that I'll go ahead and augment the CHANGELOG entry for the setenv command. Not sure that will help anyone but it'll only take me two minutes.

@zanchey
Copy link
Member

zanchey commented Jun 6, 2017

I think it should be in the release notes - @krader1961 if you don't have time to write something up I can.

@x4121
Copy link
Author

x4121 commented Jun 6, 2017

unless you're relying on undocumented behavior

Well... but it was documented. It just isn't any more.

But even if true rbenv should not have been relying on it.

Yes, but if a function behavior is documented, you can expect others to use it in that way.

Not sure that will help anyone

You mean apart from users that read the issue tracker and/or changelog? If it was documented I obviously wouldn't have looked into this and opened a ticket.

@krader1961
Copy link
Contributor

Well... but it was documented. It just isn't any more.

Excuse me? Please point me to where this was ever documented, @x4121. There is exactly one mention of it in the tutorial. It is incidentally included in the list of function names you might see if you type functions. So I don't see any justification for your claim that we had documented the broken behavior. I'm happy to eat humble pie if you can prove me wrong.

@zanchey, I added commit b361c29fe to the master branch to add more verbiage to the CHANGELOG about this.

@x4121
Copy link
Author

x4121 commented Jun 6, 2017

The description included "Alias for set -g"

function setenv --description 'Set global variable. Alias for set -g, made for csh compatibility'
(as mistydemeo wrote). Implementation detail or not, if it said it was an alias I can completely see someone prefering to write setenv over set -gx as it conveys the intention (setting an environment variable) more clearly.

The csh implementation of setenv uses a single value, but it also uses colons to separate items. Since fish separates by spaces in general, it would have made sense for a fish-implementation of setenv to allow multiple values (instead of colon separated items).

@krader1961
Copy link
Contributor

For the benefit of everyone else, @x4121 is referring to this description that used to be attached to the setenv function definition: "Set global variable. Alias for set -g, made for csh compatibility". @x4121, You are clearly twisting the intended meaning and ignoring the "csh compatibility" phrase. Not to mention that description isn't even accurate as it is a simple alias for set -gx, not set -g.

It's also a bit of a stretch to call a function description "documentation". Especially since the only way to see it means you are also looking at the, trivial, implementation and can clearly see that if you are supporting fish, rather than csh, you should simply use set -gx directly. Too, whomever wrote the fish support for rbenv clearly knew that only the old fish implementation supported multiple values and the csh version does not. So you can't claim that they were trying to have a single implementation that supported both shells. The choice to use the fish setenv wrapper and depend on its buggy behavior is bizarre in light of these facts.

@faho
Copy link
Member

faho commented Jun 6, 2017

The description included "Alias for set -g"

It also included "made for csh compatibility" - something which it now adheres to, and it previously didn't (in particular it didn't accept the colon-separated version for $PATH).

if it said it was an alias I can completely see someone prefering to write setenv over set -gx

Obviously, that actually happened.

as it conveys the intention (setting an environment variable) more clearly.

I disagree that it does, but that doesn't really matter.


So, this was unfortunate - it's annoying that we didn't catch this prior to release, in which case I'd have argued that we should at most have issued a warning.

However, it's happened now so the question is: How do we deal with this?

  • We could do nothing and wait for rbenv to make a new release (which, considering they make roughly yearly releases and have 7 commits since the last one in november, doesn't sound too great).

  • We could make a 2.6.1 release (which could also include Fix ssh completions and ssh provided hostnames #4104 and at least a few documentation changes)

  • We could tell people to use rbenv from git (considering there have been 7 commits since november, they probably didn't break too much)

  • We could tell people to use the old setenv.fish (which, considering that it has some functional additions could break other stuff)

  • We could release a "fixed" setenv.fish (with the argument count check removed)

And since I think that should still be discussed, I'm reopening this.

@faho faho reopened this Jun 6, 2017
@x4121
Copy link
Author

x4121 commented Jun 6, 2017

@krader1961 Well yes, I would call the description "documentation". I guess that's the purpose of having this description. But apart from that I guess it's useless to argue about this topic anymore due to vastly different viewpoints.

@faho The change is already implemented in rbenv's master. In the meantime it's easily fixed with a workaround. I don't think it would be good to change setenv back from it's intended behavior (unless there a lot of other programs popping out with this issue).

@krader1961
Copy link
Contributor

We could tell people to use the old setenv.fish

This is the only option I support. The only changes to our setenv are improved handling of our three magically split env vars that a rbenv user is unlikely to care about. This does not warrant a 2.6.1. release. Let alone "fixing" setenv.fish since it's current behavior is correct. I went to great pains to ensure it behaves like the csh version -- down to special casing one situation so that we would issue the same error you would get from the csh implementation rather than a slightly different message. I

@faho
Copy link
Member

faho commented Jun 6, 2017

The change is already implemented in rbenv's master. In the meantime it's easily fixed with a workaround.

Yeah, but there's the problem of getting that to users. Now, a user can install fish and rbenv, and they'll get an error. Then they'll google and maybe they'll find this thread and figure out they need to install rbenv from git instead.

That's not great. And as I've stated above, it's unlikely to change soon from the rbenv side.

This does not warrant a 2.6.1. release.

On its own, it might not. But together with #4104 and some other issues, maybe it will. I'm not saying this is emergency-release material, just that it's something to keep in mind. If we have a couple more issues of this magnitude in the next month, I'd argue for a 2.6.1.

Let alone "fixing" setenv.fish since it's current behavior is correct.

Yes, it's correct in that it behaves like csh. But it could be a bit more lenient in accepting things that it did previously.


Okay, since the consensus seems to be to leave it alone for now, I'm closing this again.

@faho faho closed this as completed Jun 6, 2017
@blimey85
Copy link

blimey85 commented Jun 7, 2017

So question, as I read through this but couldn't discern what I need to do to fix this problem. I'm on a Mac, recently upgraded Fish via HomeBrew and now I'm getting this error. Is the easiest thing to do to simply remove the latest Fish and reinstall the previous version? Would I be better off attempting to remove rbenv and install that from Github? It was also installed via Homebrew. Thanks!

@krader1961
Copy link
Contributor

@blimey85: Either install the fixed rbenv or create a file named ~/.config/fish/functions/setenv.fish with this content:

function setenv
    set -gx $argv
end

@blimey85
Copy link

blimey85 commented Jun 7, 2017

@krader1961: that did the trick. Thank you! Unfortunately, I had nuked my install so have lost my configuration but I'll get that set back up tomorrow. Supposedly everything on my system is backed up so time to test that theory. Haha. Thanks again!

@trashcan
Copy link

trashcan commented Jun 7, 2017

This affected pyenv as well, and @krader1961's work around fixes it. Thank you.

krader1961 added a commit that referenced this issue Jun 8, 2017
When 2.6.0 was released some people reported that the third-party `rbenv`
and `pyenv` commands were incorrectly depending on our `setenv` function
not behaving exactly like the csh command of the same name. Specifically,
our version had a bug. It allowed more than one value. It no longer
does so after it was rewritten so that the three auto-split vars were
correctly handled.

See issue #4103
bswinnerton added a commit to bswinnerton/dotfiles that referenced this issue Jun 13, 2017
bswinnerton added a commit to bswinnerton/dotfiles that referenced this issue Jun 13, 2017
@chilledfrogs
Copy link
Contributor

Just to add my 2 cents here: I was tearing my hair over this issue for quite some time, and at least a temporary (but apparently robust) fix which seems to work is literally making a custom init script to replace source (rbenv init -|psub):

config.fish:

status --is-interactive; and source ~/.config/fish/functions/rbenv_init_patched.fish
[...]

functions/rbenv_init_patched.fish (literally just execute rbenv init - then copy/paste into a new file and change 1st line from setenv [...] to set [...]):

set PATH /Users/<username>/.rbenv/shims/ $PATH
setenv RBENV_SHELL fish
source '/usr/local/Cellar/rbenv/1.1.0/completions/rbenv.fish'
command rbenv rehash 2>/dev/null
function rbenv
  set command $argv[1]
  set -e argv[1]

  switch "$command"
  case rehash shell
    source (rbenv "sh-$command" $argv|psub)
  case '*'
    command rbenv "$command" $argv
  end
end

This works now quite nicely for me so far, and requires no real modifications to the fish source code. If you guys notice any flaws with this method I'm eager to hear them, and sorry in advance if this is the wrong place to post this fix!

@oceanlewis
Copy link

oceanlewis commented Jun 13, 2017

@chilledfrogs If you're able to, a simpler way to skirt the issue is to install the --HEAD versions of rbenv, nodenv, and any other similar programs that rely on setenv as their upstreams have fixed the issue by removing setenv in lieu of set -gx.

For me, fixing the issue for my own use case was as simple as brew unlink rbenv and brew install rbenv --HEAD. brew unlink nodenv and brew install nodenv --HEAD for that also does it.

@chilledfrogs
Copy link
Contributor

@lewisinc I see, stupid question incoming though: why unlink and not just plain uninstall?

@oceanlewis
Copy link

@chilledfrogs not stupid question at all. The solution above is a fix someone else suggested that worked for me. uninstall will probably work, but it may be 'unsafe' in the sense that unlink will be less likely to mess with any local folders/files you have. Someone correct me if I'm wrong, but uninstall isn't guaranteed to leave things 'as is' on your filesystem. Maybe it'll be fine, I didn't test it.

@chilledfrogs
Copy link
Contributor

@lewisinc Ok, thanks a lot, seems to work :)

@ddoherty03
Copy link

And if installed with git:

    cd ~/.rbenv
    git pull origin master

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

No branches or pull requests

10 participants