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

revert(installer): use vim.o.sh instead of 'sh' #200

Merged
merged 2 commits into from
Mar 18, 2024
Merged

revert(installer): use vim.o.sh instead of 'sh' #200

merged 2 commits into from
Mar 18, 2024

Conversation

gptlang
Copy link
Contributor

@gptlang gptlang commented Mar 17, 2024

Fixes #199

Tested on Fedora 39 with NVIM v0.10.0-dev+2606-gc0549b9c4

Copy link
Contributor

github-actions bot commented Mar 17, 2024

Review Checklist

Does this PR follow the Contribution Guidelines? Following is a partial checklist:

Proper conventional commit scoping:

  • For example, fix(installer): some installer bugfix

  • Pull request title has the appropriate conventional commit prefix.

If applicable:

  • Tested
    • Tests have been added.
    • Tested manually (steps in PR description).
  • Updated documentation.

@gptlang gptlang changed the title Respect the shell set by luarock's configure script - Fix for fish shell fix(installer): Respect shell shebang of luarocks configure script Mar 17, 2024
mrcjkb
mrcjkb previously approved these changes Mar 17, 2024
Copy link
Member

@mrcjkb mrcjkb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks.

This breaks the installer for Windows (see #166).
But using the configure script on Windows might not be the best idea anyway.

@mrcjkb
Copy link
Member

mrcjkb commented Mar 17, 2024

I'll try to come up with a proper solution for Windows today, so I'll wait with merging for now.

@vhyrro
Copy link
Collaborator

vhyrro commented Mar 17, 2024

This might be a risky thing to attempt, as we can't confirm that the binary will be executable on the target filesystem. When I tested (Arch), I get a "configure is not executable" error. I do think that we should use sh instead of vim.o.sh however to make sure we're running a POSIX-compatible shell :)

@gptlang
Copy link
Contributor Author

gptlang commented Mar 18, 2024

This might be a risky thing to attempt, as we can't confirm that the binary will be executable on the target filesystem. When I tested (Arch), I get a "configure is not executable" error. I do think that we should use sh instead of vim.o.sh however to make sure we're running a POSIX-compatible shell :)

Yeah that makes sense to me. Would it make sense to hard code /bin/sh or /bin/env sh?

@mrcjkb
Copy link
Member

mrcjkb commented Mar 18, 2024

This might be a risky thing to attempt, as we can't confirm that the binary will be executable on the target filesystem. When I tested (Arch), I get a "configure is not executable" error. I do think that we should use sh instead of vim.o.sh however to make sure we're running a POSIX-compatible shell :)

Yeah that makes sense to me. Would it make sense to hard code /bin/sh or /bin/env sh?

I believe /bin/sh (or just sh) is fine. /usr/bin/env is more commonly used for bash (which is less commonly available in /bin.

@NTBBloodbath
Copy link
Member

This might be a risky thing to attempt, as we can't confirm that the binary will be executable on the target filesystem. When I tested (Arch), I get a "configure is not executable" error. I do think that we should use sh instead of vim.o.sh however to make sure we're running a POSIX-compatible shell :)

Yeah that makes sense to me. Would it make sense to hard code /bin/sh or /bin/env sh?

I believe /bin/sh (or just sh) is fine. /usr/bin/env is more commonly used for bash (which is less commonly available in /bin.

I think using /usr/bin/env sh would be best. It is the best way to maintain compatibility with any shell even in scripts for BSD systems iirc, I would go with /bin/sh but I think that would break on certain systems like Alpine, which is an edge case but it's good to keep it in mind

@mrcjkb
Copy link
Member

mrcjkb commented Mar 18, 2024

This might be a risky thing to attempt, as we can't confirm that the binary will be executable on the target filesystem. When I tested (Arch), I get a "configure is not executable" error. I do think that we should use sh instead of vim.o.sh however to make sure we're running a POSIX-compatible shell :)

Yeah that makes sense to me. Would it make sense to hard code /bin/sh or /bin/env sh?

I believe /bin/sh (or just sh) is fine. /usr/bin/env is more commonly used for bash (which is less commonly available in /bin.

I think using /usr/bin/env sh would be best. It is the best way to maintain compatibility with any shell even in scripts for BSD systems iirc, I would go with /bin/sh but I think that would break on certain systems like Alpine, which is an edge case but it's good to keep it in mind

That only matters with bash, not sh.

https://stackoverflow.com/questions/53116226/what-is-the-recommended-posix-sh-shebang

gptlang added 2 commits March 18, 2024 12:28
`/bin/sh` (or just `sh`) should exist on most if not all platforms. You aren't gonna be using `neovim` in a docker `SCRATCH` container
@gptlang
Copy link
Contributor Author

gptlang commented Mar 18, 2024

I would go with /bin/sh but I think that would break on certain systems like Alpine, which is an edge case but it's good to keep it in mind

Should we just assume that sh is available in PATH? I highly doubt anyone is running Neovim in a SCRATCH docker container.

Alternatively, we can continue using vim.o.sh and only fall back to sh if we find that the user isn't using a POSIX compatible shell.

I'm probably gonna move off fish over the coming days. The lack of POSIX compatibility is driving me nuts.

@mrcjkb mrcjkb changed the title fix(installer): Respect shell shebang of luarocks configure script revert(installer): use vim.o.sh instead of 'sh' Mar 18, 2024
@mrcjkb mrcjkb changed the title revert(installer): use vim.o.sh instead of 'sh' revert(installer): use vim.o.sh instead of 'sh' Mar 18, 2024
@mrcjkb mrcjkb merged commit eaac7b1 into nvim-neorocks:master Mar 18, 2024
2 of 3 checks passed
@mrcjkb
Copy link
Member

mrcjkb commented Mar 18, 2024

Reverts 54b67ce

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.

Fails in fish shell
4 participants