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

Allow configuring Homebrew with .env files #15787

Merged
merged 1 commit into from
Aug 1, 2023
Merged

Allow configuring Homebrew with .env files #15787

merged 1 commit into from
Aug 1, 2023

Conversation

MikeMcQuaid
Copy link
Member

For a long time people have requested some sort of configuration files for Homebrew. Now: here's the first version of that.

Similarly to how you can configure Git for a system, a repository or a user: you can configure Homebrew for a system, a prefix or a user.

The system-wide configuration file is /etc/homebrew/brew.env, the prefix-specific configuration file is $HOMEBREW_PREFIX/etc/homebrew/brew.env and the user-specific configuration file is ~/.homebrew/brew.env.

As we need to read these files from Bash in bin/brew (so they can) influence functionality ASAP: they are in a simple format that Bash can read. It may be that we have more complex array or hash data in future that's configured through JSON or YAML (most likely JSON as we use it more) and stored in a brew.json/brew.yaml file in the same directory.

As this is relying on eval in Bash which is fairly dangerous: we filter the lines with a regex to ensure we're only permitting setting HOMEBREW_* variables and nothing more.

To give a bit of power to system administrators, the HOMEBREW_SYSTEM_ENV_TAKES_PRIORITY variable can be set in /etc/homebrew/brew.env to ensure that the system-wide configuration file is loaded last and overrides any prefix or user settings.

Now that we have an actual location for configuration files, let's also change the brew livecheck watchlist configuration file to be in this directory and deprecate the existing location. As this is a developer command and the mitigation is to just move the file: we don't need to follow the normal deprecation process here.

bin/brew Outdated Show resolved Hide resolved
unset SYSTEM_ENV_TAKES_PRIORITY
# only load HOMEBREW_*=* lines
SYSTEM_HOMEBREW_ENV="$(grep -E '^HOMEBREW_[A-Z]+=.*$' "/etc/homebrew/brew.env")"
if [[ -n "${HOMEBREW_SYSTEM_ENV_TAKES_PRIORITY-}" ]]
Copy link
Member

Choose a reason for hiding this comment

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

Would be nice to check for whether this is set in /etc/homebrew/brew.env.

Copy link
Member Author

Choose a reason for hiding this comment

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

@carlocab to avoid it being set by the environment or something else? Allowing it to be set from environment makes testing a bunch easier.

Copy link
Member

@carlocab carlocab Jul 28, 2023

Choose a reason for hiding this comment

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

More to allow the administrator of /etc/homebrew/brew.env to decide whether the system env takes priority or not. Otherwise users can just do unset HOMEBREW_SYSTEM_ENV_TAKES_PRIORITY. We can take the value from the environment too if it's not set in the system env.

But I'm not bothered if you'd rather someone who needed it implemented this.

Library/Homebrew/manpages/brew.1.md.erb Outdated Show resolved Hide resolved
For a long time people have requested some sort of configuration files
for Homebrew. Now: here's the first version of that.

Similarly to how you can configure Git for a system, a repository or
a user: you can configure Homebrew for a system, a prefix or a user.

The system-wide configuration file is `/etc/homebrew/brew.env`, the
prefix-specific configuration file is
`$HOMEBREW_PREFIX/etc/homebrew/brew.env`
and the user-specific configuration file is `~/.homebrew/brew.env`.

As we need to read these files from Bash in `bin/brew` (so they can)
influence functionality ASAP: they are in a simple format that Bash
can read. It may be that we have more complex array or hash data in
future that's configured through JSON or YAML (most likely JSON as we
use it more) and stored in a `brew.json`/`brew.yaml` file in the same
directory.

As this is relying on `eval` in Bash which is fairly dangerous: we
filter the lines with a regex to ensure we're only permitting setting
`HOMEBREW_*` variables and nothing more.

To give a bit of power to system administrators, the
`HOMEBREW_SYSTEM_ENV_TAKES_PRIORITY` variable can be set in
`/etc/homebrew/brew.env` to ensure that the system-wide configuration
file is loaded last and overrides any prefix or user settings.

Now that we have an actual location for configuration files, let's also
change the `brew livecheck` watchlist configuration file to be in this
directory and deprecate the existing location. As this is a developer
command and the mitigation is to just move the file: we don't need to
follow the normal deprecation process here.
Comment on lines +105 to +107
if [[ -n "${XDG_CONFIG_HOME-}" ]]
then
HOMEBREW_USER_CONFIG_HOME="${XDG_CONFIG_HOME}/homebrew"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if [[ -n "${XDG_CONFIG_HOME-}" ]]
then
HOMEBREW_USER_CONFIG_HOME="${XDG_CONFIG_HOME}/homebrew"
if [[ -n "${HOMEBREW_XDG_CONFIG_HOME-}" ]]
then
HOMEBREW_USER_CONFIG_HOME="${HOMEBREW_XDG_CONFIG_HOME}/homebrew"

and XDG_CONFIG_HOME needs to be added to USED_BY_HOMEBREW_VARS in bin/brew.

Copy link
Member Author

Choose a reason for hiding this comment

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

@carlocab Can you explain that? I don't think either should be needed.

Copy link
Member

Choose a reason for hiding this comment

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

Env filtering:

❯ XDG_CONFIG_HOME=foo brew ruby -e 'puts ENV["XDG_CONFIG_HOME"].present?'
false

Copy link
Member Author

Choose a reason for hiding this comment

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

@carlocab We don't rely on XDG_CONFIG_HOME being set anywhere after env filtering is applied (unless I missed something), no?

Copy link
Member

Choose a reason for hiding this comment

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

Ah, sorry, I thought this was in brew.sh. 😅 That said, don't we want to have the same behaviour for the livecheck watchlist? i.e. check $XDG_CONFIG_HOME/homebrew/livecheck_watchlist.txt if XDG_CONFIG_HOME is set?

Copy link
Member Author

Choose a reason for hiding this comment

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

@carlocab that behaviour is already present. We don't pass through XDG_CONFIG_HOME because we don't want/need to. It's documented that we use it if set but that's only needed/used in brew.sh which in turn sets HOMEBREW_USER_CONFIG_HOME accordingly and that is used afterwards.

For an example:

$ export XDG_CONFIG_HOME=fish[~/OSS/Homebrew]
$ brew irb
==> Interactive Homebrew Shell
...
brew(main):001:0> ENV["HOMEBREW_USER_CONFIG_HOME"]
=> "fish/homebrew"

@MikeMcQuaid MikeMcQuaid requested a review from carlocab July 31, 2023 11:55
Copy link
Member

@carlocab carlocab left a comment

Choose a reason for hiding this comment

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

Aha, yes. I clearly did not understand the changes here. Thanks for bearing with me!

@MikeMcQuaid
Copy link
Member Author

@carlocab you're very welcome, thanks for the review! ❤️

@MikeMcQuaid MikeMcQuaid merged commit e3c0faf into Homebrew:master Aug 1, 2023
@MikeMcQuaid MikeMcQuaid deleted the env_file_configuration branch August 1, 2023 12:07
@cdalvaro
Copy link
Contributor

cdalvaro commented Aug 3, 2023

Hi!

After this change, I'm having issues running salt with brew.

Every state I try to run with brew fails with the following error message:

[salt.loaded.int.module.cmdmod:920 ][ERROR   ][737] Command 'brew' failed with return code: 1
[salt.loaded.int.module.cmdmod:922 ][ERROR   ][737] stdout: /opt/homebrew/bin/brew: line 109: HOME: unbound variable

I've checked how salt runs brew commands, and this is how it does it:

https://github.com/saltstack/salt/blob/4e8b77df671fd756970fe4fb08122fba9b47c50b/salt/modules/mac_brew_pkg.py#L112-L115

    if runas:
        _cmd = ["sudo -i -n -H -u {} -- ".format(runas)]
    _cmd = _cmd + [salt.utils.path.which("brew")] + list(cmd)
    _cmd = " ".join(_cmd)

It apparently tries to set the HOME env variable by calling sudo with -H. But looking at the error message it doesn't seem to work.

However, I've not been able to reproduce the error in any other more isolated way.

Update: As a temporary fix, I have checkout my Homebrew installation to point to the latest release:

git -C ${fetch --all
git -C ${HOMEBREW_PREFIX} checkout 4.1.3

@MikeMcQuaid
Copy link
Member Author

@cdalvaro I'll work on a fix for this tomorrow.

@cdalvaro
Copy link
Contributor

cdalvaro commented Aug 3, 2023

Great!!! Thank you very much! Let me know if I can help you!

@MikeMcQuaid
Copy link
Member Author

@cdalvaro Will be fixed by #15818

@github-actions github-actions bot added the outdated PR was locked due to age label Sep 4, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 4, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants