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

Update Launcher.php - call sh with absolute path #362

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

nivadis
Copy link

@nivadis nivadis commented Nov 18, 2024

use absolute path for sh command

use absolute path

Signed-off-by: Valentin Conrad <[email protected]>
@nivadis nivadis changed the title Update Launcher.php Update Launcher.php - call sh with absolute path Nov 18, 2024
@blizzz
Copy link
Member

blizzz commented Nov 18, 2024

why would we want to do this?

@nivadis
Copy link
Author

nivadis commented Nov 19, 2024

for me as example: the nixos nextcloud-cron doesn't have the valid $PATH to find sh.

but this will also be fixed hopefully NixOS/nixpkgs#356988
afaik the $PATH containing the location of sh is needed for POSIX compatibility

@blizzz
Copy link
Member

blizzz commented Nov 19, 2024

I think it is fair to assume sh is present and the location based on the $PATH env var should always be preferred over a hard coded and assumed path.

What would be more acceptable is to check whether command -v sh results in a known path and stick with simple sh, and test for an existing /bin/sh only as fallback.

The most preferred solution though is that the nix package provides a path to the sh bin.

Copy link
Contributor

github-actions bot commented Dec 3, 2024

Hello there,
Thank you so much for taking the time and effort to create a pull request to our Nextcloud project.

We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process.

Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6

Thank you for contributing to Nextcloud and we hope to hear from you soon!

(If you believe you should not receive this message, you can add yourself to the blocklist.)

@nivadis
Copy link
Author

nivadis commented Dec 5, 2024

The fallback to /bin/sh sounds good to me, but i agree the issue should also be fixed on the nix side.

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

Successfully merging this pull request may close these issues.

2 participants