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

cmd/shellenv: set XDG_DATA_DIRS on Linux #18326

Merged
merged 1 commit into from
Sep 17, 2024
Merged

cmd/shellenv: set XDG_DATA_DIRS on Linux #18326

merged 1 commit into from
Sep 17, 2024

Conversation

notfirefox
Copy link
Contributor

@notfirefox notfirefox commented Sep 14, 2024

Some programs like vapigen might not work correctly, when XDG_DATA_DIRS does not include $HOMEBREW_PREFIX/share. Instead of requiring the user to manually adjust the shell environment, we can set XDG_DATA_DIRS as part of cmd/shellenv, so that it is sufficient to run brew shellenv on any particular system.

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes? Here's an example.
  • Have you successfully run brew style with your changes locally?
  • Have you successfully run brew typecheck with your changes locally?
  • Have you successfully run brew tests with your changes locally?

Fixes #18317

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.

Seems reasonable to me, though I don't know how to review the PowerShell code.

Also wondering if we should just set this unconditionally on Linux.

Library/Homebrew/cmd/shellenv.sh Outdated Show resolved Hide resolved
@notfirefox
Copy link
Contributor Author

Also wondering if we should just set this unconditionally on Linux.

I think that should be fine. Although we should consider the following from the XDG Base Directory Specification.

If $XDG_DATA_DIRS is either not set or empty, a value equal to /usr/local/share/:/usr/share/ should be used.

Therefore we might want to do something like this:

export XDG_DATA_DIRS="${HOMEBREW_PREFIX}/share:${XDG_DATA_DIRS:-/usr/local/share:/usr/share}"

@carlocab
Copy link
Member

I think that should be fine. Although we should consider the following from the XDG Base Directory Specification.

Yes, we should follow that if we set it, but that doesn't seem to be much simpler than what we're already doing now. Fine with leaving it as is if it's enough to silence the warning from brew doctor.

Some programs like `vapigen` might not work correctly, when `XDG_DATA_DIRS`
does not include `$HOMEBREW_PREFIX/share`. Instead of requiring the user
to manually adjust the shell environment, we can set `XDG_DATA_DIRS` as
part of `cmd/shellenv`, so that it is sufficient to run `brew shellenv`
on any particular system.

Fixes #18317
@notfirefox notfirefox marked this pull request as ready for review September 17, 2024 12:08
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.

Thanks!

@MikeMcQuaid MikeMcQuaid merged commit 335d370 into Homebrew:master Sep 17, 2024
27 checks passed
@MikeMcQuaid
Copy link
Member

Thanks again @notfirefox!

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.

Consider setting XDG_DATA_DIRS in shellenv on Linux
3 participants