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

fish-shell: update to 3.4.1. #36121

Merged
merged 1 commit into from Mar 29, 2022
Merged

Conversation

triallax
Copy link
Contributor

@triallax triallax commented Mar 13, 2022

Testing the changes

  • I tested the changes in this PR: YES

If you use an older Fish version as your shell and update to 3.4.0, you'll get the following (I believe harmless) error after the upgrade command finishes running and in some other instances:

/usr/share/fish/functions/fish_title.fish (line 7): $(...) is not supported. In fish, please use '(prompt_hostname)'.
        and set ssh "[$(prompt_hostname | string sub -l 10)]"
                      ^
from sourcing file /usr/share/fish/functions/fish_title.fish
in command substitution
source: Error while reading file “/usr/share/fish/functions/fish_title.fish”
To be clear this does not happen when running the updated Fish 3.4.0)

Am I supposed to handle that somehow? Fixed in 3.4.1.

Local build testing

  • I built this PR locally for my native architecture, x86_64-glibc

@zen0bit
Copy link
Contributor

zen0bit commented Mar 14, 2022

Looks like I found solution

Maybe add patch for it?
Never do that ...

change
"[$(prompt_hostname | string sub -l 10)]"
to
"["(prompt_hostname | string sub -l 10 | string collect)"]"

no error for me after change...

@triallax
Copy link
Contributor Author

triallax commented Mar 14, 2022

The error only occurs because Fish 3.4.0 introduces a new syntax that the script in question was modified to use in Fish 3.4.0, and older Fish versions don't recognize that syntax. Thus, any Fish sessions started after the update will not be affected. I think applying such a patch is not a good idea, but I'll leave that choice to the maintainers.

@sgn sgn requested a review from ericonr March 17, 2022 15:43
@Vistaus
Copy link

Vistaus commented Mar 19, 2022

I also don't think applying such a patch is a good idea. Just restart your fish shell.

@paper42
Copy link
Member

paper42 commented Mar 20, 2022

Is this something that will happen only in specific circumstances or for all users? If this would happen to all users, we could add an install message.

@triallax
Copy link
Contributor Author

@paper42 well, not strictly all users it seems. From https://fishshell.com/docs/current/cmds/fish_title.html:

This requires that your terminal supports programmable titles and the feature is turned on.

I'm not sure about this, but it probably also wouldn't affect those who have a custom fish_title function (as shown in the example in the URL above). However, it's not like we know how many Fish users aren't affected, so it may be safe to assume that the majority will face that error.

@paper42
Copy link
Member

paper42 commented Mar 21, 2022

However, it's not like we know how many Fish users aren't affected, so it may be safe to assume that the majority will face that error.

In that case let's add an install message, we can remove it with the next update.

@triallax triallax marked this pull request as draft March 21, 2022 14:55
@Chocimier
Copy link
Member

Upsteam patched out new syntax, i would prefer to import that.
fish-shell/fish-shell@c5a8764

@triallax
Copy link
Contributor Author

@Chocimier guess I'll do that.

@triallax triallax changed the title fish-shell: update to 3.4.0. fish-shell: update to 3.4.1. Mar 26, 2022
@triallax
Copy link
Contributor Author

I updated the PR to Fish 3.4.1, which fixes that issue (see https://github.com/fish-shell/fish-shell/releases/tag/3.4.1).

@triallax triallax marked this pull request as ready for review March 26, 2022 10:37
@Chocimier Chocimier merged commit 57b32d6 into void-linux:master Mar 29, 2022
@triallax triallax deleted the fish-3.4.0 branch March 29, 2022 17:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants