borgbackup / homebrew-tap

borgbackup packages for macOS homebrew users
30 stars 7 forks source link

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

Closed ThomasWaldmann closed 3 years ago

ThomasWaldmann commented 3 years ago

@sunknudsen ^^^

sunknudsen commented 3 years ago

@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 commented 3 years ago

Sure, edit it! :-)

sunknudsen commented 3 years ago

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

sunknudsen commented 3 years ago

More changes upstream... depends_on :osxfuse will be deprecated shortly (see https://github.com/Homebrew/brew/issues/9401#issuecomment-738007106).

sunknudsen commented 3 years ago

@m3nu 👆

m3nu commented 3 years ago

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

sunknudsen commented 3 years ago

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 commented 3 years ago

renaming the formula to |borgbackup-fuse|?

Not sure.

In borg 1.2 you need either llfuse or pyfuse3.

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

Alternatively, only offer 1 way and decide it for the user (then it could be -fuse).

m3nu commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

@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 https://github.com/libfuse/pyfuse3/issues/32).

ThomasWaldmann commented 3 years ago

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.

sunknudsen commented 3 years ago

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 commented 3 years ago

i put it yesterday on twitter/mastodon...

ThomasWaldmann commented 3 years ago

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

sunknudsen commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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

sunknudsen commented 3 years ago

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

m3nu commented 3 years ago

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 commented 3 years ago

@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 https://github.com/borgbackup/borg/issues/5522#issuecomment-738508169).

m3nu commented 3 years ago

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

sunknudsen commented 3 years ago

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

sunknudsen commented 3 years ago

@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 commented 3 years ago

4768151e4ee4515f232a76dfe89488b0672d840b 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 commented 3 years ago

@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 commented 3 years ago

Maybe like this:

sunknudsen commented 3 years ago

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

sunknudsen commented 3 years ago

@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 commented 3 years ago

looks good, merged!