Navigation Menu

Skip to content
This repository has been archived by the owner on Jul 4, 2023. It is now read-only.

Make "pgroonga" 0.9.0 to be optional resource in postgresql formula #44739

Closed

Conversation

cosmo0920
Copy link
Contributor

Revised #41174 with optional resource technique.

@cosmo0920 cosmo0920 force-pushed the postgresql-pgroonga-0.9.0-merge branch from 63cd957 to d156773 Compare October 8, 2015 13:49
@cosmo0920
Copy link
Contributor Author

This PR should solve sandbox issue, but test-bot CI reports tests failure.
How do I fix them?

depends_on "openssl"
depends_on "readline"
depends_on "libxml2" if MacOS.version <= :leopard # Leopard libxml is too old
depends_on :python => :optional
depends_on "groonga" => :build if build.with? "pgroonga"
Copy link
Member

Choose a reason for hiding this comment

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

If two variables are the same block them:

if build.with? "pgroonga"
  depends_on "groonga" => :build
  depends_on "pkg-config" => :build
end

@DomT4
Copy link
Member

DomT4 commented Oct 8, 2015

This PR should solve sandbox issue, but test-bot CI reports tests failure.
How do I fix them?

On El Capitan it's using the deprecated system OpenSSL instead of ours. It probably needs something like --with-openssl=#{Formula["openssl"].opt_bin}. Check the configure switches and see what's available on that.

The other failure is a sandbox failure elsewhere, with the test trying to use a configuration file we no longer permit:

Testing pgpool-ii
==> Using the sandbox
==> /usr/local/Cellar/pgpool-ii/3.4.2/bin/pg_md5 --md5auth pool_passwd --config-file /usr/local/etc/pgpool.conf.sample
ERROR: pid 81709: initializing pool password, failed to open file:"/usr/local/etc/pool_passwd"
Error: pgpool-ii: failed
Failed executing: /usr/local/Cellar/pgpool-ii/3.4.2/bin/pg_md5 --md5auth pool_passwd --config-file /usr/local/etc/pgpool.conf.sample

@cosmo0920 cosmo0920 force-pushed the postgresql-pgroonga-0.9.0-merge branch from c74f13a to ea43ed2 Compare October 8, 2015 15:30
@cosmo0920
Copy link
Contributor Author

Check the configure switches and see what's available on that.

--with-openssl option is available, but it does not nothing passed there.
I fixed to pass Formula["openssl"].opt_prefix into this option.

# Now build and install pgroonga as optional resource
if build.with? "pgroonga"
resource("pgroonga").stage do
ENV.append_path "PATH", Formula["postgresql"].bin
Copy link
Member

Choose a reason for hiding this comment

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

It should be just bin here. You don't need Formula["postgresql"] to point itself.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've confirmed that building pgroonga succeeded using bin here.

@xu-cheng
Copy link
Member

xu-cheng commented Oct 8, 2015

Can you add a test for this option?

Also our CI won't test non default option. So you should test this locally.

@DomT4
Copy link
Member

DomT4 commented Oct 8, 2015

Also our CI won't test non default option.

I'm not sure it's worth adding tests for non-default options. We've refused to do this several times over the last couple months even because it opens the floodgates to having a test element for every option even though we'd never use it. Has our position changed here?

@xu-cheng
Copy link
Member

xu-cheng commented Oct 8, 2015

We've refused to do this several times over the last couple months

I was unaware this. Please ignore my comment on adding new test. But still this option need to be tested locally.

@DomT4
Copy link
Member

DomT4 commented Oct 8, 2015

I'm fine if we want to have that team discussion; just didn't know if I'd missed something somewhere :)

This reverts commit ea43ed2.

with-openssl does not assume argument.... :/
@cosmo0920
Copy link
Contributor Author

Check the configure switches and see what's available on that.

--with-openssl option is available, but it does not nothing passed there.
I fixed to pass Formula["openssl"].opt_prefix into this option.

Hmm, --with-openssl does not accept arguments.... :/

@DomT4
Copy link
Member

DomT4 commented Oct 11, 2015

@BrewTestBot test this please

@DomT4
Copy link
Member

DomT4 commented Oct 12, 2015

This is passing the CI now. The OpenSSL issue got fixed with the version bumps & changes merged yesterday. How does pgroonga know where to install, out of interest? I noticed there's no prefix argument being passed anywhere.

@cosmo0920
Copy link
Contributor Author

How does pgroonga know where to install, out of interest? I noticed there's no prefix argument being passed anywhere.

Ah..., yes. Is this not suitable for Homebrew's policy?
PGroonga's makefile calculates suitable installation location.
In this situation, pgroonga.so is installed in $(brew -prefix)/Cellar/postgresql/<version>/lib and pgroonga.sql is installed in $(brew -prefix)/Cellar/postgresql/<version>/share/postgresql/extension/.

@DomT4
Copy link
Member

DomT4 commented Oct 12, 2015

It's fine if that's the case. Just wanted to check it was installing into the prefix and not a generic /usr/local directory or such.

@DomT4
Copy link
Member

DomT4 commented Oct 12, 2015

Merged in 022faf8. Thank you for another contribution to Homebrew @cosmo0920 🌟

@DomT4 DomT4 closed this in 022faf8 Oct 12, 2015
@cosmo0920
Copy link
Contributor Author

Thanks! I'm appreciate to make to be pgroonga optional resource in postgresql formula!

@cosmo0920 cosmo0920 deleted the postgresql-pgroonga-0.9.0-merge branch October 12, 2015 04:32
@Homebrew Homebrew locked and limited conversation to collaborators Jul 10, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants