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.

Sign up for GitHub

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

Jump to bottom

feat: provide explicit feedback for new MacOS rsync/openrsync issue #172

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

feat: provide explicit feedback for new MacOS rsync/openrsync issue #172

wants to merge 2 commits into from

Conversation

jsstevenson
Copy link
Contributor

@jsstevenson jsstevenson commented Jan 13, 2025 •
edited
Loading

close #171

  • If subprocess calls return an error, check to see if the rsync binary looks like rsync, and give a more descriptive error message if so
  • Update requirements in the README

@jsstevenson jsstevenson marked this pull request as ready for review January 28, 2025 19:15
@jsstevenson jsstevenson requested a review from a team as a code owner January 28, 2025 19:15
korikuzma
korikuzma reviewed Jan 29, 2025
View reviewed changes
README.md Outdated
@@ -100,6 +100,8 @@ Reading a sequence repository requires several Python packages, all of which are
available from pypi. Installation should be as simple as `pip install
biocommons.seqrepo`.

Acquiring SeqRepo snapshots using the CLI requires an [rsync](https://github.com/RsyncProject/rsync) binary to be available on the system $PATH. Note that [openrsync](https://www.openrsync.org/), which now ships with new MacOS installs, does not support all required functions. Mac users should install rsync from [HomeBrew](https://formulae.brew.sh/formula/rsync) and ensure that it's available on the $PATH.
Copy link

Choose a reason for hiding this comment

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

@jsstevenson what do you get when you run which rsync?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jsstevenson what do you get when you run which rsync?

I wound up just installing mine via homebrew so it's in /opt/homebrew/bin/. In the latest MacOS version, openrsync appears to be located at /usr/bin/rsync

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, you can also use --rsync-exe to point to a binary even if it's not on your $PATH, added a note about that

@jsstevenson
Copy link
Contributor Author

jsstevenson commented Jan 31, 2025 •
edited
Loading

Ah, I see that the CLI points directly to /usr/bin/rsync by default rather than just falling back on the PATH. That seems problematic IMO, I don't know if that would be a standard or expected location across distros (vs say /usr/local/bin/), and I think it defies expectations a bit to effectively ignore how the PATH is arranged. I guess either the language above needs to be recrafted to advise use of the path option, or the default value should change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@korikuzma korikuzma korikuzma left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Rsync non-zero exit status when rsync'ing seqrepo
2 participants
@jsstevenson @korikuzma