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(bug): create /usr/local/bin if not present #614

Merged
merged 2 commits into from
Mar 29, 2022

Conversation

schmikei
Copy link
Contributor

@schmikei schmikei commented Mar 28, 2022

Description of Changes

  • Some installations(notably default M1 macOS 12) do not have the folder /usr/local/bin present when they run the installer. So this PR addresses this by ensuring the directory exists before linking the installed binary to the $PATH

Tested on linux by removing the /usr/local/bin and rerunning the installer whereas on main it failed.

From my understanding it is the responsibility of the third party app in the macOS ecosystem to create the folder if it wishes to add things to /usr/local/bin https://discussions.apple.com/thread/252495484

Please check that the PR fulfills these requirements

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)
  • CI passes

@schmikei schmikei requested a review from a team as a code owner March 28, 2022 20:42
@schmikei schmikei requested a review from antonblock March 28, 2022 20:42
@codecov
Copy link

codecov bot commented Mar 28, 2022

Codecov Report

❗ No coverage uploaded for pull request base (main@4b9cc75). Click here to learn what that means.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #614   +/-   ##
=======================================
  Coverage        ?   74.98%           
=======================================
  Files           ?      133           
  Lines           ?    10089           
  Branches        ?        0           
=======================================
  Hits            ?     7565           
  Misses          ?     2054           
  Partials        ?      470           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4b9cc75...1ffe1d0. Read the comment docs.

@jsirianni jsirianni self-requested a review March 28, 2022 21:25
@BinaryFissionGames
Copy link
Contributor

BinaryFissionGames commented Mar 28, 2022

Do we even want to put a link in this directory if it doesn't exist? I imagine this is to make it so that stanza is on your path, does creating the directory and linking actually do that on the affected systems?

jsirianni
jsirianni previously approved these changes Mar 28, 2022
README.md Outdated
@@ -104,6 +104,7 @@ Restart Stanza: `sudo systemctl restart stanza`.
- Single command install, requires the `curl` command
- Stanza will automatically be running as a service
- On Linux, Stanza will be running as the `root` user. On Macos, Stanza will be running as your current user.
- `sudo` may be required if user running installer needs permission to write to installation locations and linking to `/usr/local/bin`.
Copy link
Member

Choose a reason for hiding this comment

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

Sudo is always required for Linux, and will be required for macos if the user does not have write permission to /usr/local/bin.

@schmikei
Copy link
Contributor Author

Do we even want to put a link in this directory if it doesn't exist? I imagine this is to make it so that stanza is on your path, does creating the directory and linking actually do that on the affected systems?

I agree with this generally, just am thinking about any current users that maybe built automation around expecting it being in /usr/local/bin. That is the only argument I have for the link in the first place, perhaps in the future we make a more definitive call on whether or not installations add to /usr/local/bin in the first place, but I just wanted to not introduce any potential regressions at this moment.

@schmikei schmikei dismissed stale reviews from BinaryFissionGames and jsirianni via 1ffe1d0 March 28, 2022 21:49
@BinaryFissionGames BinaryFissionGames self-requested a review March 28, 2022 21:53
@schmikei schmikei merged commit 48c23de into main Mar 29, 2022
@schmikei schmikei deleted the accomodate-missing-/usr/local/bin branch March 29, 2022 13:03
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.

4 participants