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 missing Tarsnap command-line utilities #202

Closed

Conversation

shawwn
Copy link
Contributor

@shawwn shawwn commented Aug 23, 2018

Closes #168

I'm not absolutely certain this will fix #168, but I'm reasonably
certain.

I ran into the bug again tonight on a clean macOS laptop. The core
issue seems to be that this line is returning an empty string for
fresh installations of Tarsnap:

_tarsnapDir = Utils::findTarsnapClientInPath("", true);

Rather than hunt down why it's empty, I just fall back to
/usr/local/bin if it is.

@shinnok
Copy link
Contributor

shinnok commented Aug 23, 2018

@shawwn The reason is that /usr/local/bin is not in the default $PATH on macOS, even after installing Homebrew, one has to add it manually to PATH via bash rcs.

@shinnok
Copy link
Contributor

shinnok commented Aug 23, 2018

Need to be thoughtful with adding default fall-back to /usr/local/bin, since Tarsnap GUI has to be considerate of all Unices, not just macOS. :)

@shawwn
Copy link
Contributor Author

shawwn commented Aug 23, 2018

People still use unix?

Mostly joking.

@shawwn
Copy link
Contributor Author

shawwn commented Aug 23, 2018

The reason is that /usr/local/bin is not in the default $PATH on macOS, even after installing Homebrew, one has to add it manually to PATH via bash rcs.

I think /usr/local/bin is on every macOS $PATH, even for fresh installs. (I created a new user and verified.)

https://stackoverflow.com/questions/9832770/where-is-the-default-terminal-path-located-on-mac

The value of /etc/paths is:

/usr/local/bin
/usr/bin
/bin
/usr/sbin
/sbin

@shinnok
Copy link
Contributor

shinnok commented Aug 23, 2018

Not for GUI desktop apps.

@shawwn
Copy link
Contributor Author

shawwn commented Aug 23, 2018

Ah, this was assuming tarsnap-gui would be migrating to a brew cask Tarsnap formula. I believe it takes care of adding /usr/local/bin to the PATH.

@gperciva
Copy link
Member

Heh, this exposes an amusing series of errors (mostly on my part). I was puzzled as to why this wasn't failing the tests, but I discovered:

  • the automated tests aren't enabled on Travis-CI because there's something wrong with their installation of Qt. I haven't seen it happen on Ubuntu 16.04 xenial, so I was waiting in the hopes that they'd get xenial images soon (since it's been more than 2 years since its release!). I might set up my own test suite on a raspberry pi.

  • the automated tests do check that a fake-dir produces no binary, but it does this by changing the value of the appDataPathLineEdit widget. That triggers the validateAdvancedSetupPage() function, which has its own Utils::findTarsnapClientInPath() on line 250, which wipes out the previous manual setting. If we apply the proposed fix to that line, the test does fail properly.

  • The test should fail because we use a non-blank _tarsnapDir to indicate that binary is present, so having a fall-back value of /usr/local/bin disrupts that.

If the brew cask takes care of /usr/local/bin, then I'd rather rely on that instead of messing around with Utils::findTarsnapClientInPath() and/or changing setupwizard.cpp. I'll double-check this once the cask is out there.

@shawwn
Copy link
Contributor Author

shawwn commented Aug 23, 2018

If the brew cask takes care of /usr/local/bin

Ah, small correction -- the fallback to /usr/local/bin is necessary on macOS regardless of whether it was installed via brew cask or not. When I did a user testing session yesterday, the user ran into this issue even though I'd installed Tarsnap.app via the new brew cask mechanism.

Alternatively, we could fix findTarsnapClientInPath to return /usr/local/bin on macOS (or at least not return blank).

@gperciva
Copy link
Member

I have an idea for findTarsnapClientInPath(), which I'll implement and put up. I'd rather not add more stuff to 1.0.2, but I think this is important enough to qualify.

@gperciva
Copy link
Member

Thanks for this PR! I'm closing it because it's been obsoleted by #204, but the discussion here was instrumental in creating that PR.

@gperciva gperciva closed this Aug 25, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Command-line utilities not found when tarsnap is installed through Homebrew
3 participants