Skip to content

readme: be more specific, explain what one gets#1

Merged
ThomasWaldmann merged 2 commits into
masterfrom
ThomasWaldmann-patch-1
Dec 4, 2020
Merged

readme: be more specific, explain what one gets#1
ThomasWaldmann merged 2 commits into
masterfrom
ThomasWaldmann-patch-1

Conversation

@ThomasWaldmann
Copy link
Copy Markdown
Member

No description provided.

@ThomasWaldmann
Copy link
Copy Markdown
Member Author

@sunknudsen ^^^

@sunknudsen
Copy link
Copy Markdown
Member

@ThomasWaldmann Thanks for the PR. borgbackup-llfuse depends on FUSE for macOS therefore when using this formula vs borgbackup, one has to install the dependency using brew install --cask osxfuse first.

This dependency was previously avoided given brew install borgbackup was packaged as a bottle automatically by Homebrew (vs borgbackup-llfuse which installs from source).

Would you like me to edit your PR?

@ThomasWaldmann
Copy link
Copy Markdown
Member Author

Sure, edit it! :-)

@sunknudsen
Copy link
Copy Markdown
Member

@ThomasWaldmann Edit done... before getting this project out to the world, perhaps we should consider renaming the formula to borgbackup-fuse?

@sunknudsen
Copy link
Copy Markdown
Member

More changes upstream... depends_on :osxfuse will be deprecated shortly (see Homebrew/brew#9401 (comment)).

@sunknudsen
Copy link
Copy Markdown
Member

@m3nu 👆

@m3nu
Copy link
Copy Markdown
Contributor

m3nu commented Dec 3, 2020

Depending on Casks seems to be a mess either way. I'm looking at some alternatives.

@sunknudsen
Copy link
Copy Markdown
Member

I agree... depending on casks is essentially not supported anymore by Homebrew. That being said, installing osxfuse using brew install --cask osxfuse before running brew install borgbackup-llfuse (which I believe should be renamed to borgbackup-fuse) works fine.

@ThomasWaldmann
Copy link
Copy Markdown
Member Author

ThomasWaldmann commented Dec 3, 2020 via email

@m3nu
Copy link
Copy Markdown
Contributor

m3nu commented Dec 3, 2020

Since the formulas conflict anyways, why not just call it the same? Docs only say:

If your formulae have the same name as Homebrew/homebrew-core formulae they cannot be installed side-by-side. ^1

@sunknudsen
Copy link
Copy Markdown
Member

I don't dislike the fact open source purists (I don't mean that in a negative way) may choose to install borgbackup over borgbackup-fuse as the latter depends on proprietary code. My gut feeling is using a different name might help avoid confusion.

@sunknudsen
Copy link
Copy Markdown
Member

@ThomasWaldmann

If both shall be offered, keeping -llfuse in the name would be good.

I agree and that is why I used the llfuse suffix in the first place. That being said, given llfuse is deprecated, my gut feeling is we might want to switch to pyfuse3 when possible (see libfuse/pyfuse3#32).

@ThomasWaldmann
Copy link
Copy Markdown
Member Author

we'll start trying pyfuse3 after 1.2 is out. if it is available good enough, works good enough and everybody is happy, we can drop llfuse.

Comment thread README.md Outdated
Comment thread README.md
@sunknudsen
Copy link
Copy Markdown
Member

we'll start trying pyfuse3 after 1.2 is out. if it is available good enough, works good enough and everybody is happy, we can drop llfuse.

Got it... I recommend renaming the formula to borgbackup-fuse before the formula is made public... I am planning on publishing an episode tomorrow so ideally we would be set on the name today. @ThomasWaldmann @m3nu Thoughts? If we proceed with the name change, I would simply rename the formula... aside from us, I don't think others will have had a chance to switch from borgbackup.

@ThomasWaldmann
Copy link
Copy Markdown
Member Author

i put it yesterday on twitter/mastodon...

@ThomasWaldmann
Copy link
Copy Markdown
Member Author

i'm fine with both pkg names, choose what you like. :-)

@sunknudsen
Copy link
Copy Markdown
Member

i put it yesterday on twitter/mastodon...

@ThomasWaldmann I guess we should deprecate borgbackup-llfuse and keep both for a few months then? Thanks for your guidance btw... This is my second collab on an open source project with considerable reach... I want to make sure I don't make naive mistakes.

@ThomasWaldmann
Copy link
Copy Markdown
Member Author

i just pointed users to the ticket (which points to the repo here).

so guess it's better to just change it, should not affect many (and even if, it doesn't matter if it breaks now or in a few months).

@sunknudsen
Copy link
Copy Markdown
Member

@m3nu We appear to have consensus to rename the package to borgbackup-fuse. Cool?

@sunknudsen
Copy link
Copy Markdown
Member

Standby for @m3nu's feedback. PR is ready for final review... I also fixed the depends_on :osxfuse deprecation issue.

@m3nu
Copy link
Copy Markdown
Contributor

m3nu commented Dec 4, 2020

Either name is fine, as long as the product works.

I'll try the update today. We may also want to run brew test via Github actions.

@sunknudsen
Copy link
Copy Markdown
Member

@ThomasWaldmann Ready to merge? @m3nu will likely submit another PR with tests and other optimizations but reports the formula also worked at his end (see borgbackup/borg#5522 (comment)).

@m3nu
Copy link
Copy Markdown
Contributor

m3nu commented Dec 4, 2020

We can do it like that. I based everything off this branch. So can merge my commits on top of it. It adds:

  • Lint formula to pass brew audit
  • Add back tests to formula
  • Run install and tests in GH actions.
  • Add some extra tests for borg mount

@sunknudsen
Copy link
Copy Markdown
Member

Nice additions @m3nu. Never used GitHub actions. Looks promising!

@sunknudsen
Copy link
Copy Markdown
Member

@ThomasWaldmann What governance would you like us to follow in the context of merging PRs? This PR is ready to go and I would like to start recording an episode so ideally we would merge it shortly.

@ThomasWaldmann
Copy link
Copy Markdown
Member Author

4768151 seems to rename a file and do significant changes to the contents of that file (the ruby code of the formula).

That is almost never a good idea because it is hard to see. If something needs to be renamed, only do the rename and commit.

Besides that, it is unrelated to the scope of this PR, which was to improve the readme.

@sunknudsen
Copy link
Copy Markdown
Member

@ThomasWaldmann I have to admit this initial PR got messy. Sorry about that. What is the best way to split it into multiple PRs?

@ThomasWaldmann
Copy link
Copy Markdown
Member Author

Maybe like this:

  • keep a copy of the modified formula at some other place.
  • copy the unmodified formula over the renamed one and then git add / git commit --amend / git push -f or so.
  • finish this PR and modify the formula in another PR as needed.

@sunknudsen
Copy link
Copy Markdown
Member

@ThomasWaldmann OK, I will try that... on it!

@sunknudsen
Copy link
Copy Markdown
Member

@ThomasWaldmann I believe I cleaned up the PR mess... I feel like such a newbie... This was my first time creating conflicting pull requests all at the same time. Looking forward to your feedback.

@ThomasWaldmann ThomasWaldmann merged commit 86ee695 into master Dec 4, 2020
@ThomasWaldmann ThomasWaldmann deleted the ThomasWaldmann-patch-1 branch December 4, 2020 17:48
@ThomasWaldmann
Copy link
Copy Markdown
Member Author

looks good, merged!

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.

3 participants