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

fix: replaced file existence check for shell check #1445

Merged
merged 3 commits into from Feb 1, 2022

Conversation

afonsojramos
Copy link
Member

In theory, fixes #1435.

@ririxidev @theRealPadster

@afonsojramos afonsojramos self-assigned this Feb 1, 2022
@afonsojramos afonsojramos added the 🐛 bug Something isn't working label Feb 1, 2022
@theRealPadster
Copy link
Member

theRealPadster commented Feb 1, 2022

$SHELL doesn't work in Fish. For that matter, the rest of the syntax isn't compatible with Fish anyways, so the script can not be run with Fish. (You'd need to run under a different shell, but it would be nice if spicetify was available in the Fish$PATH afterwards.) I believe that $SHELL is meant to output the currently running shell, which would mean that it only adds to the $PATH of a single shell, which we don't want.

@afonsojramos
Copy link
Member Author

the rest of the syntax isn't compatible with Fish anyways

I thought #1426 made it work, what do you mean? Or are you simply referring to if [[ -f "$HOME/.config/fish/config.fish" ]]? I can roll that back.

which would mean that it only adds to the $PATH of a single shell, which we don't want.

I disagree here. This should be the default behaviour.

@theRealPadster
Copy link
Member

theRealPadster commented Feb 1, 2022

That commit enables it to add to the fish path, but the script itself will not run under fish because fish isn't posix-compatible and the syntax isn't identical.

I don't see the harm in adding it to multiple shells if the user has multiple shells set up. I generally use fish, but have zsh on MacOS as well, and have experimented with some different ones. This is why I think it's still a net positive to include support for the fish path even though the script can't be run from fish

Context:

fish, the friendly interactive shell, is a commandline shell intended to be interactive and user-friendly.

fish is intentionally not fully POSIX compliant, it aims at addressing POSIX inconsistencies (as perceived by the creators) with a simplified or a different syntax. This means that even simple POSIX compliant scripts may require some significant adaptation or even full rewriting to run with fish.

https://wiki.archlinux.org/title/fish

@afonsojramos
Copy link
Member Author

In that case I'll rollback the fish change. Either way, the installation must be run in a POSIX compliant shell, so it will then work with fish after installation.

@afonsojramos
Copy link
Member Author

@theRealPadster now we should be good to cover all edge cases.

@theRealPadster
Copy link
Member

@theRealPadster now we should be good to cover all edge cases.

Awesome, looks good to me, thanks!

@rxri
Copy link
Member

rxri commented Feb 1, 2022

Yeah looks good now ig. Now there should be no issues with zsh users etc.

@afonsojramos afonsojramos merged commit 0d72f0f into master Feb 1, 2022
@afonsojramos afonsojramos deleted the install-shell-fix branch February 1, 2022 17:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛 bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

macOS installer is broken
3 participants