libcouchbase 2.8.4

[avsej]

• Have you followed the guidelines for contributing?
• Have you checked that there aren't other open pull requests for the same formula update/change?
• Have you built your formula locally with brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
• Does your build pass brew audit --strict <formula> (after doing brew install <formula>)?

[ilovezfs]

@avsej why not depend on the snappy formula?

[avsej]

Because right now we only tested against snappy-1.1.1, and snappy-1.1.7 homebrew is shipping does not define its version correctly at least. This breaks our build right now. Also see https://bugzilla.redhat.com/show_bug.cgi?id=1527850

[ilovezfs]

That may be because we build snappy with CMake. I'd like to use the formula regardless of whether that specific version has been tested.

[avsej]

snappy-1.1.7 ships broken header file

[ilovezfs]

Have you reported this upstream? Why is it that it works for

folly.rb:26:  depends_on "snappy"
leveldb.rb:19:  depends_on "snappy"
ppsspp.rb:21:  depends_on "snappy"
redis-leveldb.rb:20:  depends_on "snappy"
rocksdb.rb:15:  depends_on "snappy"
snappystream.rb:18:  depends_on "snappy"
snzip.rb:16:  depends_on "snappy"
sparkey.rb:19:  depends_on "snappy"
wiredtiger.rb:15:  depends_on "snappy"

if that is the case?

[avsej]

Because these packages don't use version macros in the header.

[ilovezfs]

We'll need to find some workaround for that then and get it reported upstream.

[avsej]

Workaround is to use verified snappy we have in the source package. I will report the issue to upstream.

[ilovezfs]

I'm not comfortable using a static vendored version when we have a formula that nine other formulae are using fine.

[avsej]

So you would rather change this formula and likely break it, than accept it with bundled snappy?

You can take this patch to fix snappy formula: https://github.com/google/snappy/pull/61.patch

[ilovezfs]

@avsej #21925

[avsej]

@ilovezfs I have updated the formula. Now it will compile snappy if the end user will decide to do it. And by default it will not use compression at all.

[ilovezfs]

@avsej #21925 is shipped so you can just depend on snappy unconditionally now.

[avsej]

No, I cannot depend on that snappy. I would rather make this feature optional now. In its current shape, is this formula acceptable?

[ilovezfs]

Why not? It builds as expected now.

[avsej]

Because we have not tested with this snappy. I'm not actively using MacOS, so we can change formula to use external snappy later.

[ilovezfs]

@avsej The difference is in patch level. I don't think we need to worry about a problem here. I'm happy to revert it later if you identify an actual problem but we should be optimistic that it'll be fine.

[avsej]

Right now compression feature is not publicly available on Couchbase Server, so I would rather stick to static optional snappy.

[ilovezfs]

I'm 👎 on adding new options here, let alone options for static libraries. So we'll need to go without compression or use the external library.

[avsej]

What wrong with options? It will make it easier for people to opt-in for features.

[ilovezfs]

They result in source builds instead of using the prebuilt binary, but if you want to make it

depends_on "snappy" => :optional

I'd be OK with that for now.

[avsej]

I removed snappy support from the formula.

[ilovezfs]

OK.

[ilovezfs]

Thanks for the upgrade @avsej! Shipped.