-
Notifications
You must be signed in to change notification settings - Fork 3.7k
chore: add homebrew recipe #26697
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
base: main
Are you sure you want to change the base?
chore: add homebrew recipe #26697
Conversation
@bnpfeife I could use your review given your experience with these recipes. @jdstrand Also requesting your review given this does some process work similar to the direct install script, and I know we needed some security adjustments there, so being cognizant of that here. Note: this if for 3.3.0 for testing, but we'll need it for 3.4.0 when we actually publish. |
@peterbarnett03 - hey, is there a reason you picked ruby? It isn't used in the codebase (or really at InfluxData any more). Could this be shell (preferable) or maybe python (if shell doesn't work)? If not, we can make it work, but it would be better if it is a language we are already using in the codebase. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a few points of feedback below. I think mainly the audit part might be a blocker, as I'm not sure if the recipe will be accepted with audit failures.
Documentation
I think it would be helpful to add some doc comments to each method defined in this file which explain the purpose of each method.
Audit failures
I ran brew audit
on this and there are several issues that likely should be addressed before submitting:
brew audit
warnings:
brew audit local/test/influxdb3-core
local/test/influxdb3-core
* line 2, col 9: Description shouldn't start with the formula name.
* line 20, col 1: Trailing whitespace detected.
* line 26, col 1: Trailing whitespace detected.
* line 32, col 1: Trailing whitespace detected.
* line 34, col 7: Favor modifier `if` usage when having a single-line body. Another good alternative is the usage of control flow `&&`/`||`.
* line 38, col 1: Trailing whitespace detected.
* line 41, col 1: Trailing whitespace detected.
* line 44, col 1: Trailing whitespace detected.
* line 46, col 1: Trailing whitespace detected.
* line 48, col 1: Trailing whitespace detected.
* line 50, col 1: Trailing whitespace detected.
* line 53, col 1: Trailing whitespace detected.
* line 55, col 1: Trailing whitespace detected.
* line 58, col 1: Trailing whitespace detected.
* line 66, col 1: Trailing whitespace detected.
* line 68, col 1: Trailing whitespace detected.
* line 70, col 1: Trailing whitespace detected.
* line 75, col 1: Trailing whitespace detected.
* line 80, col 1: Trailing whitespace detected.
* line 86, col 1: Trailing whitespace detected.
* line 89, col 1: Trailing whitespace detected.
* line 98, col 1: Trailing whitespace detected.
* line 102, col 1: Trailing whitespace detected.
* line 107, col 1: Trailing whitespace detected.
* line 114, col 1: Trailing whitespace detected.
* line 122, col 1: Trailing whitespace detected.
* line 125, col 1: Trailing whitespace detected.
* line 127, col 1: Trailing whitespace detected.
* line 139, col 1: Trailing whitespace detected.
* line 144, col 1: Trailing whitespace detected.
* line 147, col 1: Trailing whitespace detected.
* line 161, col 1: Trailing whitespace detected.
* line 172, col 1: Trailing whitespace detected.
* line 180, col 1: Trailing whitespace detected.
* line 186, col 1: Trailing whitespace detected.
* line 189, col 1: Trailing whitespace detected.
* line 198, col 1: Trailing whitespace detected.
* line 201, col 1: Trailing whitespace detected.
* line 206, col 1: Extra empty line detected at method body beginning.
* line 207, col 12: Don't use ANSI escape codes in the caveats.
* line 208, col 17: Don't use ANSI escape codes in the caveats.
* line 209, col 11: Don't use ANSI escape codes in the caveats.
* line 210, col 10: Don't use ANSI escape codes in the caveats.
* line 240, col 8: Trailing whitespace detected.
* line 255, col 5: Use `assert_path_exists <path_to_file>` instead of `assert_predicate bin/"influxdb3", :exist?`
* line 257, col 1: Trailing whitespace detected.
* line 258, col 5: Use `assert_path_exists <path_to_file>` instead of `assert_predicate bin/"influxdb3-core", :exist?`
* line 260, col 1: Trailing whitespace detected.
* line 263, col 1: Trailing whitespace detected.
* line 264, col 5: Use `assert_path_exists <path_to_file>` instead of `assert_predicate etc/"influxdb3/influxdb3.conf", :exist?`
* line 268, col 1: Trailing whitespace detected.
* line 269, col 5: Use `assert_path_exists <path_to_file>` instead of `assert_predicate var/"lib/influxdb3", :exist?`
* line 270, col 5: Use `assert_path_exists <path_to_file>` instead of `assert_predicate var/"lib/influxdb3/README.txt", :exist?`
* line 271, col 1: Trailing whitespace detected.
* line 272, col 5: Use `assert_path_exists <path_to_file>` instead of `assert_predicate var/"log/influxdb3", :exist?`
* line 282, col 1: Trailing whitespace detected.
* line 284, col 1: Trailing whitespace detected.
* line 294, col 4: Final newline missing.
* Non-executables were installed to "/opt/homebrew/opt/influxdb3-core/bin".
The offending files are:
/opt/homebrew/opt/influxdb3-core/bin/python
Error: 59 problems in 1 formula detected.```
Testing
Some guidance on how to test this would be helpful, but I used Claude to help me set it up locally and it was able to install it. Here are the sequence of commands that were used to test it:
# create local test tap:
brew tap-new local/test
# copy the recipe to test formula location:
cp influxdb3-core.rb /opt/homebrew/Library/Taps/local/homebrew-test/Formula/
# install from source:
brew install --build-from-source local/test/influxdb3-core
# test the recipe:
brew test local/test/influxdb3-core
Claude thinks that brew test
was successful, but I don't think so. The test appears to be installing and starting the server (which succeeds); however, it then issues a curl
command:
curl -s -f http://127.0.0.1:55301/health || echo 'Server listening'
which fails because the server requires authentication to the /health
endpoint. I think that for the sake of this test, you could use the --without-auth
argument for the started test server, or even better, use the --disable-authz health
to disable auth for just the /health
endpoint to facilitate the test without having to create a token.
Homebrew recipes/formulas need to be written in Ruby |
FWIW - re: the |
Fixed all the warnings except one which continues to complain that:
Can't find info on it on Google -- Claude says it's fine and "normal" (though my gut tells me that's a hallucination). I've tried other workarounds like symlinking and changing to lib, but that causes other issues. |
Ah, interesting, I reckon that is the python that is installed for the processing engine? That may actually be indicative of a problem. I tried starting the installed server with a
If I compile the |
Hmmmm odd. What if you try |
Yeah it works when running as a service. This is probably okay then. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you've covered my requests. It would be good to get Claude to add some doc comments for our future selves.
Recall that the If this location is a problem for
(the If it would help
With that, if |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Marking 'Request changes'; see in-line.
influxdb3-core.rb
Outdated
(bin/"python/lib").mkpath | ||
|
||
python_lib = python_prefix/"lib/libpython3.13.dylib" | ||
(bin/"python/lib/libpython3.13.dylib").make_symlink(python_lib) if python_lib.exist? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should remove this. influxdb3
needs to use the libpython
version it was compiled against. If this works at all today, it is by luck that there is a libpython3.13.dylib
on the system (not all macOS systems will have this version) and the design of the processing engine is that it is NOT dependent on the system for python (if we changed the python version we want to use, we'd break these systems). Further, the system libpython3.13.dylib
could potentially subtly incompatible with the one that we compiled against.
The brew
file needs to unconditionally ship libpython
in a location that influxdb3
can find it; when it does so, there is no reason to have fallback code (a user removing files from the brew install location can't possibly expect the code to work as designed; we need to just error).
@jdstrand Appreciate the notes. I've made updates to how it requires Python to only use the bundled Python. Review requested. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See inline for how to fix the final problem. Basically, we install the official binary and the python directory into libexec
, which is something like /opt/homebrew/Cellar/influxdb3-core/3.3.0/libexec
. Then create a wrapper in bin
to call out to it via the unversioned opt symlink:
% cat /opt/homebrew/bin/influxdb3
#!/bin/bash
...
exec "/opt/homebrew/opt/influxdb3-core/libexec/influxdb3" "$@"
% influxdb3 serve --object-store memory --node-id n0 --plugin-dir /tmp/plugins
2025-08-21T22:11:37.308697Z INFO influxdb3_lib::commands::serve: InfluxDB 3 Core server starting node_id=n0 git_hash=02d7ee1e6fec5b62debbe862881562e451624de6 version=3.3.0 uuid=68ca191c-b9af-44b6-9598-1d70cdbb622a num_cpus=11
...
2025-08-21T22:11:37.423195Z INFO influxdb3_server: startup time: 114ms address=0.0.0.0:8181
...
No need to call install_name_tool
and codesign
since the python/
directory is where the influxdb3
binary expects it.
Separately, if you are planning on doing the equivalent for enterprise, it would be good to set influxdb3-core
as a variable at the top of the file and have all other occurrences of influxdb3-core
reference the var instead. In this manner, the file could be shared with enterprise and only need one thing to change.
end | ||
|
||
# Generate a default configuration file with common settings | ||
def generate_default_config(config_dir) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hiltontj - I mentioned this in another issue, but what @peterbarnett03 is doing here suggests we should support a proper configuration file in addition to cli args and env vars. Whenever we update our deb/rpm scripts for the systemd unit, a configuration file would be useful there as well (though, admittedly, systemd has an EnvironmentFile
that can be used similarly to how this brew formula is doing things).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do currently support configuration via a .env
file, but I am not entirely sure how one points the service at a particular .env
file. My understanding is it needs to be in the directory the binary is run from.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hiltontj - the type of configuration file I'm talking about would be something like /etc/influxdb3/influxdb.conf
where the serve
process would look at it. Eg:
- https://docs.influxdata.com/influxdb/v1/administration/config/#using-the-configuration-file
- https://docs.influxdata.com/enterprise_influxdb/v1/administration/configure/config-data-nodes/#data-node-configuration-settings
- https://docs.influxdata.com/influxdb/v2/reference/config-options/?t=JSON#influxdb-configuration-file
This lists configuration preference: https://docs.influxdata.com/influxdb/v2/reference/config-options/?t=JSON#configuration-precedence
1.x and 2.x use have various file formats for the config file, but I'm not suggesting anything particular. Using an env
file format with an option like influxdb3 serve -c|--config /path/to/config/file
would be fine IME. Then we could ship a nicely formatted example file that comes from official repos in our packages rather then all the different package formats coming up with something bespoke. This would ease maintenance, provide a consistent user experience and the config file could be self-documenting, which would help users (it could be organized in a way that is easy for folks to read too).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not entirely sure how one points the service at a particular .env file
if you can forgive an interloper's presence (I was just looking at how cli flags are setup in core), claude suggests
- dotenv crate looks in the current directory and parents -- doesn't seem to have a way to point it at a file
- manually parsing for
-c|--config
, loading env vars, and then parsing with clap - using clap's
try_parse
and only access-c|--config|INFLUXDB3_CONFIG_FILE_PATH
if it is present, load the env vars, and then parsing again with claps::parse - using the
config
crate, but it looks tricky to combine that with clap since you want the env vars defined before clap::parse is called.
looks slightly unpleasant either way especially since we might like an env var like INFLUXDB3_CONFIG_FILE_PATH
to work too!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using an
env
file format with an option likeinfluxdb3 serve -c|--config /path/to/config/file
would be fine IME.
Agreed. env
file format would also make it consistent with the .env
files that are currently supported, so we can provide a nicely formatted/commented example that covers both use cases.
looks slightly unpleasant either way especially since we might like an env var like INFLUXDB3_CONFIG_FILE_PATH to work too!
😭
Okay, I can open an issue for a configuration file feature.
@jdstrand Ok, incorporated your requested changes. Let me know if good for approval (though, I may wind up merging this into our new forked Homebrew repo instead to push from here). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for incorporating the changes! I'm approving as-is, but see inline comment to have brew audit --strict ...
come back clean.
version "3.3.0" | ||
license any_of: ["Apache-2.0", "MIT"] | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you remove one of these 2 newlines, then brew audit --strict ...
will come back clean.
Adds Homebrew recipe for InfluxDB 3 Core.
Due to how Homebrew works, we can't pass in variables for starting info, like node-id. Due to this, the recipe creates a default config with "node0" and outputs this info in the caveats. I don't think we can make it much simpler, akin to the old approach of just running influxd, but this gets pretty close.