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

rethinkdb 2.4.2 #100256

Closed
wants to merge 1 commit into from
Closed

rethinkdb 2.4.2 #100256

wants to merge 1 commit into from

Conversation

cho-m
Copy link
Member

@cho-m cho-m commented Apr 27, 2022

Created by brew bump


Created with brew bump-formula-pr.

@BrewTestBot BrewTestBot added bump-formula-pr PR was created using `brew bump-formula-pr` macos-only Formula depends on macOS no ARM bottle Formula has no ARM bottle labels Apr 27, 2022
@cho-m cho-m force-pushed the bump-rethinkdb-2.4.2 branch from 554c166 to 6e34ea7 Compare April 27, 2022 17:41
@BrewTestBot BrewTestBot added no Linux bottle Formula has no Linux bottle and removed macos-only Formula depends on macOS labels Apr 27, 2022
@cho-m cho-m force-pushed the bump-rethinkdb-2.4.2 branch 2 times, most recently from e8e1add to 77b7a6a Compare April 27, 2022 19:30
@cho-m cho-m added do not merge in progress Stale bot should stay away labels Apr 27, 2022
@cho-m cho-m force-pushed the bump-rethinkdb-2.4.2 branch from 77b7a6a to 7cabb8a Compare April 27, 2022 21:12
@cho-m cho-m removed do not merge in progress Stale bot should stay away labels Apr 27, 2022
@Bo98
Copy link
Member

Bo98 commented Apr 27, 2022

This is still using Python 2 on macOS:

* Error: missing PYTHON. Install it or specify the full path with PYTHON=

@cho-m
Copy link
Member Author

cho-m commented Apr 27, 2022

This is still using Python 2 on macOS:

* Error: missing PYTHON. Install it or specify the full path with PYTHON=

I can extend env var to

ENV["PYTHON"] = which("python3") if !OS.mac? || MacOS.version >= :catalina

Not entirely sure our current Python policy, i.e. do we prefer system Python 3 now? I would say Python bindings should use Homebrew Python, but build-time/runtime is still vague.

@Bo98
Copy link
Member

Bo98 commented Apr 27, 2022

It is a bit vague, yeah.

Definitely Homebrew Python for any bindings or anything that does any sort of pip install/virtualenv. Really most runtime usages to be honest unless it's for some minor script or something that can dynamically pick up any Python on the path like npm.

For build-time, it probably doesn't matter. Can go with system (or it's really from the dev tools) Python there if there's no tight versioning requirements. Just be careful it's not secretly installing bindings is all I say and definitely make sure < Catalina is considered.

@cho-m cho-m force-pushed the bump-rethinkdb-2.4.2 branch from 7cabb8a to 1eaa1d3 Compare April 27, 2022 22:06
Comment on lines 34 to 35
# rethinkdb requires that protobuf be linked against libc++
# but brew's protobuf is sometimes linked against libstdc++
Copy link
Member

Choose a reason for hiding this comment

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

This comment must be at least 10 years old.

Copy link
Member Author

@cho-m cho-m Apr 27, 2022

Choose a reason for hiding this comment

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

The test.proto used to check functionality hasn't been updated and causes configure error:

Test protobuf:                  error
* Error: Unable to compile sample protobuf file. Try running ./configure with the --fetch protoc option
[libprotobuf WARNING google/protobuf/compiler/parser.cc:649] No syntax specified for the proto file: mk/gen/protoc/test.proto. Please use 'syntax = "proto2";' or 'syntax = "proto3";' to specify a syntax version. (Defaulted to proto2 syntax.)
In file included from ./mk/gen/protoc//test.pb.cc:4:
In file included from ./mk/gen/protoc/test.pb.h:23:
In file included from /usr/local/include/google/protobuf/io/coded_stream.h:154:
In file included from /usr/local/include/google/protobuf/stubs/common.h:48:
* Aborting configure

The version they fetch is actually the same as our current version (3.19.4), but they bypass check in this case.

https://github.com/rethinkdb/rethinkdb/blob/v2.4.2/configure#L1083

Copy link
Member

Choose a reason for hiding this comment

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

Fair enough, this isn't a blocker.

Though that error doesn't really say anything because it's just a warning, and they limit # of lines output: https://github.com/rethinkdb/rethinkdb/blob/9090794e52078e9ba10bf34e4f6c399f0f71c29e/configure#L416

Copy link
Member Author

Choose a reason for hiding this comment

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

I increased errors and looks like test is failing on default c++ standard:

/usr/local/include/google/protobuf/stubs/port.h:123:2: error: "Protobuf requires at least C++11."

Could try a run with ENV.cxx11.

Copy link
Member

Choose a reason for hiding this comment

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

Worth a shot I'd say.

@cho-m cho-m force-pushed the bump-rethinkdb-2.4.2 branch from 1eaa1d3 to 560c52f Compare April 27, 2022 23:38
@BrewTestBot
Copy link
Member

🤖 A scheduled task has triggered a merge.

@cho-m cho-m deleted the bump-rethinkdb-2.4.2 branch April 28, 2022 03:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bump-formula-pr PR was created using `brew bump-formula-pr` no ARM bottle Formula has no ARM bottle no Linux bottle Formula has no Linux bottle
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants