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.

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

stellar-core 9.1.0 (new formula) #24443

Conversation

Projects
None yet
3 participants
@superstructor
Copy link
Contributor

commented Feb 23, 2018

  • 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>)?

Formula/stellar-core.rb Outdated
head "https://github.com/stellar/stellar-core.git", :using => :git

depends_on "pkg-config" => :run
depends_on "libsodium" => :run

This comment has been minimized.

Copy link
@fxcoudert

fxcoudert Feb 23, 2018

Member

Remove :run for libsodium, it is by default

This comment has been minimized.

Copy link
@superstructor

superstructor Feb 24, 2018

Author Contributor

Done, thanks.

Formula/stellar-core.rb Outdated

depends_on "pkg-config" => :run
depends_on "libsodium" => :run
depends_on "libtool" => :run

This comment has been minimized.

Copy link
@fxcoudert

fxcoudert Feb 23, 2018

Member

Are you sure pkg-config and libtool are really required at runtime?

This comment has been minimized.

Copy link
@superstructor

superstructor Feb 24, 2018

Author Contributor

Your correct, only required for build, thanks.

Formula/stellar-core.rb Outdated
depends_on "libtool" => :run
depends_on "autoconf" => :build
depends_on "automake" => :build
depends_on "postgresql" => :recommended

This comment has been minimized.

Copy link
@fxcoudert

fxcoudert Feb 23, 2018

Member

Let's make this mandatory, by removing => :recommended. Fewer options are better for our users, who benefit from prebuilt binaries.

This comment has been minimized.

Copy link
@superstructor

superstructor Feb 24, 2018

Author Contributor

Done, thanks.

Formula/stellar-core.rb Outdated
else
args << "--disable-postgres"
end
system "./configure", *args

This comment has been minimized.

Copy link
@fxcoudert

fxcoudert Feb 23, 2018

Member

After option is removed, inline all arguments after system "./configure" with no need for the args array

This comment has been minimized.

Copy link
@superstructor

superstructor Feb 24, 2018

Author Contributor

Done, thanks.

@fxcoudert
Copy link
Member

left a comment

Thanks for the pull request!

Formula/stellar-core.rb Outdated
desc "The backbone of the Stellar (XLM) network"
homepage "https://www.stellar.org/"
url "https://github.com/stellar/stellar-core.git",
:using => :git,

This comment has been minimized.

Copy link
@DomT4

DomT4 Feb 24, 2018

Contributor

The :using => :git here and below are both redundant.

This comment has been minimized.

Copy link
@superstructor

superstructor Feb 24, 2018

Author Contributor

Done, thanks.

@superstructor superstructor force-pushed the superstructor:stellar-core-9.1.0-new-formula branch Feb 24, 2018

@superstructor

This comment has been minimized.

Copy link
Contributor Author

commented Feb 24, 2018

Thank you so much for the rapid response and helpful feedback! I've made the requested changes and retested.

@superstructor superstructor force-pushed the superstructor:stellar-core-9.1.0-new-formula branch Feb 24, 2018

class StellarCore < Formula
desc "The backbone of the Stellar (XLM) network"
homepage "https://www.stellar.org/"
url "https://github.com/stellar/stellar-core.git",

This comment has been minimized.

Copy link
@fxcoudert

fxcoudert Feb 25, 2018

Member

We would prefer to build from a tarball (https://github.com/stellar/stellar-core/archive/v9.1.0.tar.gz) as this makes full use of Homebrew's caching mechanism.

This comment has been minimized.

Copy link
@DomT4

DomT4 Feb 26, 2018

Contributor

Looks like there's submodules, which might not be part of the standard archive tarball? Can't remember off the top of my head 😓.

This comment has been minimized.

Copy link
@DomT4

DomT4 Feb 26, 2018

Contributor

No, those folders are empty in the archive tarball. So would either need the git clone or a fuller release tarball, I guess.

This comment has been minimized.

Copy link
@superstructor

superstructor Feb 26, 2018

Author Contributor

Yes unfortunately doesn't appear possible to build from tarball at the moment. That is what I had attempted first. For the moment, this works and is not the only formula resorting to git. I'll also do a follow up pull request to improve this as soon as the situation changes.

Formula/stellar-core.rb Outdated

depends_on "libsodium"
depends_on "postgresql"
depends_on "pkg-config" => :build

This comment has been minimized.

Copy link
@fxcoudert

fxcoudert Feb 25, 2018

Member

Build deps first, ordered alphabetically among them. Then runtime deps.

This comment has been minimized.

Copy link
@superstructor

superstructor Feb 26, 2018

Author Contributor

Done, thanks.

@superstructor superstructor force-pushed the superstructor:stellar-core-9.1.0-new-formula branch to 3080243 Feb 26, 2018

@fxcoudert

This comment has been minimized.

Copy link
Member

commented Feb 26, 2018

Thanks @superstructor for the pull request, and congratulations on your first Homebrew contribution!

@fxcoudert fxcoudert closed this in 0a9b758 Feb 26, 2018

@Homebrew Homebrew locked and limited conversation to collaborators May 4, 2018

@superstructor superstructor deleted the superstructor:stellar-core-9.1.0-new-formula branch Sep 23, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.