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

libbitcoin 3.3.0 (new formula) #16771

Closed
wants to merge 1 commit into from
Closed

Conversation

sugarjig
Copy link
Contributor

@sugarjig sugarjig commented Aug 14, 2017

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

This is libbitcoin, a C++ library for Bitcoin development. There are actually two new formula in this pull request: libbitcoin and libbitcoin-secp256k1.

libbitcoin depends on libbitcoin-secp256k1. The repo is actually a fork of the one maintained by the Bitcoin Core team. While I would have preferred to use the source repo, they have not created a tagged release. The fork, maintained by the libbitcoin team, does have a tagged release. Because of this, I am defining secp256k1 as a resource and building it as part of this formula.

@ilovezfs
Copy link
Contributor

If secp256k1 is just needed as a dependency of libbitcoin, then you can put it in a resource block, rather than using a separate formula and a fork.

@ilovezfs ilovezfs added the new formula PR adds a new formula to Homebrew/homebrew-core label Aug 14, 2017
@sugarjig
Copy link
Contributor Author

sugarjig commented Aug 15, 2017

Thanks for the suggestion! I'm a bit confused though. I'm looking at the resource method documentation. It says I can access the file from the install method, but it doesn't say exactly how. Is it just a file that will be at the root of #buildpath? Then do I just untar it and do the build/install dance I'm otherwise doing in the other formula?

Edit: I totally missed the instance method. I'll look into that now.

@sugarjig sugarjig force-pushed the libbitcoin branch 2 times, most recently from a6e1832 to d6b6e9a Compare August 15, 2017 12:57
@sugarjig
Copy link
Contributor Author

@ilovezfs I'm using a resource for the secp256k1 dependency, and all the checks are passing.

system "./autogen.sh"
system "./configure", "--disable-dependency-tracking",
"--disable-silent-rules",
"--prefix=#{prefix}",
Copy link
Contributor

Choose a reason for hiding this comment

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

This should use libexec instead of prefix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, so that's what it's for.

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 made this change, but I have been doing some testing. The libbitcoin shared library actually links to it, which tells me that it needs to be installed on your system in order for libbitcoin to work. Also, I plan on following up with more formulas for other libbitcoin libraries, and they also depend on secp256k1. I've started the process of making these formulae, and I'm having to define secp256k1 as a resource in all of them. Is there another way to deal with this, or can I change it back to #{prefix} so that it is installed?

Hopefully that makes sense.

Here's the output from otool. libbitcoin links to the shared lib under libexec.

$ otool -L /usr/local/lib/libbitcoin.dylib 
/usr/local/lib/libbitcoin.dylib:
	/usr/local/opt/libbitcoin/lib/libbitcoin.0.dylib (compatibility version 1.0.0, current version 1.0.0)
	/usr/local/opt/boost/lib/libboost_chrono-mt.dylib (compatibility version 0.0.0, current version 0.0.0)
	/usr/local/opt/boost/lib/libboost_date_time-mt.dylib (compatibility version 0.0.0, current version 0.0.0)
	/usr/local/opt/boost/lib/libboost_filesystem.dylib (compatibility version 0.0.0, current version 0.0.0)
	/usr/local/opt/boost/lib/libboost_iostreams-mt.dylib (compatibility version 0.0.0, current version 0.0.0)
	/usr/local/opt/boost/lib/libboost_locale-mt.dylib (compatibility version 0.0.0, current version 0.0.0)
	/usr/local/opt/boost/lib/libboost_log-mt.dylib (compatibility version 0.0.0, current version 0.0.0)
	/usr/local/opt/boost/lib/libboost_program_options-mt.dylib (compatibility version 0.0.0, current version 0.0.0)
	/usr/local/opt/boost/lib/libboost_regex-mt.dylib (compatibility version 0.0.0, current version 0.0.0)
	/usr/local/opt/boost/lib/libboost_system.dylib (compatibility version 0.0.0, current version 0.0.0)
	/usr/local/opt/boost/lib/libboost_thread-mt.dylib (compatibility version 0.0.0, current version 0.0.0)
	/usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 1238.60.2)
	/usr/local/Cellar/libbitcoin/3.3.0/libexec/lib/libsecp256k1.0.dylib (compatibility version 1.0.0, current version 1.0.0)
	/usr/lib/libc++.1.dylib (compatibility version 1.0.0, current version 307.5.0)

Copy link
Contributor

Choose a reason for hiding this comment

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

@sugarjig you'll need to get them to tag a release.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair enough. I'm able to get the other formulas working by defining secp256k1 as a resource, but those libraries end up linking to the version installed in their own cellar, which is fine.

Unless you have any more change requests, I'm all done! Thanks for the help!

# Normally pkgconfig metadata for a dependency is available
# /usr/local/lib/pkgconfig, but Homebrew doesn't install it until the
# whole formula is complete.
ENV["PKG_CONFIG_PATH"] = "#{lib}/pkgconfig"
Copy link
Contributor

Choose a reason for hiding this comment

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

ENV.prepend_path "PKG_CONFIG_PATH", libexec/"lib/pkgconfig"

and please move it outside of the block.

}
EOS
system ENV.cxx, "-std=c++11",
"test.cpp",
Copy link
Contributor

Choose a reason for hiding this comment

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

@sugarjig
Copy link
Contributor Author

@ilovezfs Made the changes you requested. Let me know what you think!

Copy link
Contributor

@JCount JCount left a comment

Choose a reason for hiding this comment

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

system "make", "install"
end

# Normally pkgconfig metadata for a dependency is available at
Copy link
Contributor

Choose a reason for hiding this comment

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

The comment isn't needed.

@ilovezfs
Copy link
Contributor

@sugarjig did you open an issue to request a tag?

@sugarjig
Copy link
Contributor Author

@ilovezfs There is an existing issue here: bitcoin-core/secp256k1#286. I added a comment, but It's been open for 2 years, so I don't think they're going to add a tag any time soon. I think the solution we have here, where we define it as a resource, will work until they ever add one.

Add formula for libbitcoin, a C++ library for Bitcoin development (https://github.com/libbitcoin/libbitcoin). Before building, the formula downloads and builds libbitcoin's secp256k1 fork (https://github.com/libbitcoin/secp256k1). The formula includes a small C++ program for the test.
@ilovezfs
Copy link
Contributor

Thanks for your first contribution to Homebrew, @sugarjig! Without people like you submitting PRs we couldn't run this project. You rock!

@sugarjig sugarjig deleted the libbitcoin branch August 20, 2017 18:46
@Homebrew Homebrew locked and limited conversation to collaborators May 4, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
new formula PR adds a new formula to Homebrew/homebrew-core
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants