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

ARROW-4649: [C++/CI/R] Add nightly job that tests the homebrew formula #5360

Closed
wants to merge 17 commits into from

Conversation

nealrichardson
Copy link
Member

This doesn't just test brew install apache-arrow --HEAD as the ticket suggested--it brings the Homebrew formula under our source control and tests against that. This will enable us to know when we need to update the Homebrew formula due to changes in dependencies or cmake configuration.

I've been testing out this approach on a different repository: https://travis-ci.org/nealrichardson/arrow-brew/builds/568531245

@nealrichardson
Copy link
Member Author

@ursabot crossbow submit homebrew-formula

@ursabot
Copy link

ursabot commented Sep 11, 2019

AMD64 Conda Crossbow Submit (#61145) builder has been succeeded.

Revision: 2e5254d

Submitted crossbow builds: ursa-labs/crossbow @ ursabot-180

Task Status
homebrew-formula TravisCI

@nealrichardson
Copy link
Member Author

This fails, and it is correct that it fails. Compare the previous passing build I linked with https://travis-ci.org/nealrichardson/arrow-brew/builds/583867759, run on that repository just now.

The failure is

[ 19%] Generating Flight.pb.cc, Flight.pb.h, Flight.grpc.pb.cc, Flight.grpc.pb.h
cd /tmp/apache-arrow-20190911-98334-mvj5bo/build/src/arrow/flight && /usr/local/bin/protoc -I/tmp/apache-arrow-20190911-98334-mvj5bo/cpp/../format --cpp_out=/tmp/apache-arrow-20190911-98334-mvj5bo/build/src/arrow/flight /tmp/apache-arrow-20190911-98334-mvj5bo/cpp/../format/Flight.proto
cd /tmp/apache-arrow-20190911-98334-mvj5bo/build/src/arrow/flight && /usr/local/bin/protoc -I/tmp/apache-arrow-20190911-98334-mvj5bo/cpp/../format --grpc_out=/tmp/apache-arrow-20190911-98334-mvj5bo/build/src/arrow/flight --plugin=protoc-gen-grpc=/usr/local/Cellar/grpc/1.23.0_2/bin/grpc_cpp_plugin /tmp/apache-arrow-20190911-98334-mvj5bo/cpp/../format/Flight.proto
dyld: Library not loaded: /usr/local/opt/gperftools/lib/libprofiler.0.dylib
  Referenced from: /usr/local/Cellar/grpc/1.23.0_2/bin/grpc_cpp_plugin
  Reason: image not found
--grpc_out: protoc-gen-grpc: Plugin killed by signal 6.
make[2]: *** [src/arrow/flight/Flight.pb.cc] Error 1
make[2]: *** Deleting file `src/arrow/flight/Flight.pb.cc'
make[1]: *** [src/arrow/flight/CMakeFiles/flight_grpc_gen.dir/all] Error 2
make[1]: *** Waiting for unfinished jobs....
[ 19%] Built target arrow_dataset_objlib
make: *** [all] Error 2

I don't think that failure should block the merging of this patch: it might be more appropriate to fix the formula on its own ticket, just to track the issue and make clear at release time that there's been a change to the homebrew formula that we'll need to publish upstream.

Thoughts @kou @xhochy ?

@kou
Copy link
Member

kou commented Sep 12, 2019

I think that the grpc Formula in Homebrew misses the gperftools Formula dependency.

Could you send a pull request to https://github.com/homebrew/homebrew-core ?

diff --git a/Formula/grpc.rb b/Formula/grpc.rb
index dff0ba50cb..b99120414d 100644
--- a/Formula/grpc.rb
+++ b/Formula/grpc.rb
@@ -3,7 +3,7 @@ class Grpc < Formula
   homepage "https://grpc.io/"
   url "https://github.com/grpc/grpc/archive/v1.23.0.tar.gz"
   sha256 "f56ced18740895b943418fa29575a65cc2396ccfa3159fa40d318ef5f59471f9"
-  revision 2
+  revision 3
   head "https://github.com/grpc/grpc.git"
 
   bottle do
@@ -17,6 +17,7 @@ class Grpc < Formula
   depends_on "libtool" => :build
   depends_on "c-ares"
   depends_on "gflags"
+  depends_on "gperftools"
   depends_on "[email protected]"
   depends_on "protobuf"
 

@nealrichardson
Copy link
Member Author

Did we change something since August 6 that now requires gperftools? grpc hasn't changed wrt gperftools in that time. Should we ourselves just add a dependency on gperftools?

@kou
Copy link
Member

kou commented Sep 12, 2019

This is not related to the apache-arrow formula.
Because grpc_cpp_plugin provided by the grpc formula uses libprofiler.0.dylib:

$ otool -L /usr/local/Cellar/grpc/1.23.0_2/bin/grpc_cpp_plugin
/usr/local/Cellar/grpc/1.23.0_2/bin/grpc_cpp_plugin:
	/System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation (compatibility version 150.0.0, current version 1575.17.0)
	/usr/local/opt/protobuf/lib/libprotoc.20.dylib (compatibility version 21.0.0, current version 21.1.0)
	/usr/local/opt/protobuf/lib/libprotobuf.20.dylib (compatibility version 21.0.0, current version 21.1.0)
	/usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 1252.250.1)
	/usr/lib/libz.1.dylib (compatibility version 1.0.0, current version 1.2.11)
	/usr/local/opt/gperftools/lib/libprofiler.0.dylib (compatibility version 5.0.0, current version 5.18.0)
	/usr/local/opt/c-ares/lib/libcares.2.dylib (compatibility version 6.0.0, current version 6.0.0)
	/usr/lib/libc++.1.dylib (compatibility version 1.0.0, current version 400.9.4)

I think that gRPC detects gperftools automatically and gperftools is installed unexpectedly on environment that builds bottles for the grpc formula.

@nealrichardson
Copy link
Member Author

Sure. I'm just saying, I got this formula to build successfully on Travis most recently on August 6. And obviously it built fine at our last release. So do we have a theory for why this is now a problem?

@kou
Copy link
Member

kou commented Sep 12, 2019

I think that this is caused by Homebrew/homebrew-core@8defc4a . (August 24)

@kou
Copy link
Member

kou commented Sep 12, 2019

Or Homebrew/homebrew-core@f48ab37 (August 16)

@kou
Copy link
Member

kou commented Sep 12, 2019

Or Homebrew/homebrew-core@2cc1d02 (September 9)

@nealrichardson
Copy link
Member Author

So it was just luck that wherever the bottles were made before had it, and suddenly it doesn't?

@kou
Copy link
Member

kou commented Sep 12, 2019

I think that we should ask to Homebrew developers. :-)

@nealrichardson
Copy link
Member Author

👍 ok I can initiate that at Homebrew-core tomorrow (unless you want to right now); in the meantime, do you want to accept this patch? It's not like a(nother) failing nightly build is going to hold anyone up here.

@kou
Copy link
Member

kou commented Sep 12, 2019

@nealrichardson
Copy link
Member Author

I did. So I'll add to our LICENSE file then. Good catch, sorry about that.

desc "Columnar in-memory analytics layer designed to accelerate big data"
homepage "https://arrow.apache.org/"
url "https://www.apache.org/dyn/closer.cgi?path=arrow/arrow-0.14.1/apache-arrow-0.14.1.tar.gz"
sha256 "9948ddb6d4798b51552d0dca3252dd6e3a7d0f9702714fc6f5a1b59397ce1d28"
Copy link
Member

Choose a reason for hiding this comment

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

When should we update url and sha256?
If it's release time, we should update them automatically in release process.
(dev/release/00-prepare.sh?)

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed, will do.

ci/apache-arrow.rb Outdated Show resolved Hide resolved
sha256 "b5d11576669485b771d2fab149d499b41ec9d0fcda55d7f4ec98716e1e2f5c08" => :sierra
end

depends_on "autoconf" => :build
Copy link
Member

Choose a reason for hiding this comment

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

Note that we can remove this when we bundle jemalloc that has configure.

Copy link
Member Author

Choose a reason for hiding this comment

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

Cool, is there a ticket for that on which we can note this?

Copy link
Member

Choose a reason for hiding this comment

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

No.
FYI: #5365 will bundle jemalloc that has configure.

Copy link
Member Author

Choose a reason for hiding this comment

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

Since that merged, I've removed this line

@kszucs
Copy link
Member

kszucs commented Sep 12, 2019

@nealrichardson could you move every the formula under dev/tasks/brew-formulas, similarly like other packaging tasks' assets are placed under dev/tasks.

Also we need to add a license note, that we have copied code from homebrew, it has BSD-2, similarly like we reference the vendored the conda-forge recipes.

dev/tasks/tasks.yml Outdated Show resolved Hide resolved
@nealrichardson
Copy link
Member Author

@ursabot crossbow submit homebrew-formula

@ursabot
Copy link

ursabot commented Sep 13, 2019

AMD64 Conda Crossbow Submit (#61857) builder has been succeeded.

Revision: 58bfe1c

Submitted crossbow builds: ursa-labs/crossbow @ ursabot-188

Task Status
homebrew-formula TravisCI

@nealrichardson
Copy link
Member Author

Ok, feedback addressed, and Homebrew-core has resolved the grpc issue, so the build passes now.

I was able to add the version updating to the release script, but I didn't see where it was appropriate to update the sha after the release happens. But even if that remains a manual step, we're still better off than we were before.

Copy link
Member

@kou kou left a comment

Choose a reason for hiding this comment

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

@wesm Do you know any workflow when we send a patch to non ASF projects from our repository?
We'll send a patch to Homebrew from a formula file in this repository. Should we keep the original license (BSD 2-Clause License) of a formula file in this repository? Or should we relicense a formula to Apache License 2.0 in this repository and relicense to BSD 2-Clause License again?
I think that we should keep the original license (BSD 2-Clause License) to avoid relicensing.

LICENSE.txt Outdated Show resolved Hide resolved
dev/tasks/tasks.yml Show resolved Hide resolved
ci/apache-arrow.rb Outdated Show resolved Hide resolved
dev/release/00-prepare.sh Outdated Show resolved Hide resolved
@nealrichardson
Copy link
Member Author

@ursabot crossbow submit homebrew-cpp

@ursabot
Copy link

ursabot commented Sep 17, 2019

AMD64 Conda Crossbow Submit (#62952) builder has been succeeded.

Revision: f155a27

Submitted crossbow builds: ursa-labs/crossbow @ ursabot-195

Task Status
homebrew-cpp TravisCI

@nealrichardson
Copy link
Member Author

@ursabot crossbow submit homebrew-cpp-autobrew

@ursabot
Copy link

ursabot commented Sep 17, 2019

AMD64 Conda Crossbow Submit (#62954) builder has been succeeded.

Revision: f155a27

Submitted crossbow builds: ursa-labs/crossbow @ ursabot-196

Task Status
homebrew-cpp-autobrew TravisCI

@nealrichardson
Copy link
Member Author

Revised, PTAL

@nealrichardson
Copy link
Member Author

Ok, I've addressed @kou's feedback and the lint/release tests pass: https://travis-ci.org/apache/arrow/jobs/587562985

ci/apache-arrow.rb Outdated Show resolved Hide resolved
dev/tasks/tasks.yml Outdated Show resolved Hide resolved
@kszucs
Copy link
Member

kszucs commented Sep 20, 2019

@nealrichardson it looks good to me, I only have concerns with the file organization.

@nealrichardson
Copy link
Member Author

@ursabot crossbow submit homebrew-cpp homebrew-cpp-autobrew

@ursabot
Copy link

ursabot commented Sep 20, 2019

AMD64 Conda Crossbow Submit (#64046) builder has been succeeded.

Revision: 1d6512b

Submitted crossbow builds: ursa-labs/crossbow @ ursabot-210

Task Status
homebrew-cpp TravisCI
homebrew-cpp-autobrew TravisCI

@nealrichardson
Copy link
Member Author

@ursabot crossbow submit homebrew-cpp-autobrew

@ursabot
Copy link

ursabot commented Sep 20, 2019

AMD64 Conda Crossbow Submit (#64067) builder has been succeeded.

Revision: ea3f9b5

Submitted crossbow builds: ursa-labs/crossbow @ ursabot-211

Task Status
homebrew-cpp-autobrew TravisCI

@nealrichardson
Copy link
Member Author

https://travis-ci.org/nealrichardson/arrow/jobs/587687890 lint/release test passing again. Once homebrew-cpp-autobrew passes, this is good to go.

@nealrichardson
Copy link
Member Author

@kou we are finally ready to merge here. Thanks for all of your feedback!

Copy link
Member

@kou kou left a comment

Choose a reason for hiding this comment

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

+1
Thanks! I'll merge this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants