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

shellenv: Homebrew-ize XDG_DATA_DIRS if set #17932

Closed
wants to merge 1 commit into from

Conversation

gromgit
Copy link
Member

@gromgit gromgit commented Aug 1, 2024

XDG_DATA_DIRS is already set at the system level on many Linux distros, which negates the glib patch that quietly adds Homebrew's share dir to that search path. It's therefore necessary for brew shellenv to prepend to XDG_DATA_DIRS if it's set.

Closes Homebrew/homebrew-core#179217.

  • 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?

`XDG_DATA_DIRS` is already set at the system level on many Linux distros,
which negates the `glib` patch that quietly adds Homebrew's `share`
dir to that search path. It's therefore necessary for `brew shellenv`
to prepend to `XDG_DATA_DIRS` if it's set.

Closes Homebrew/homebrew-core#179217.
Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

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

Makes sense, thanks again @gromgit!

Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

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

I note that check_xdg_data_dirs in diagnostic.rb seems to be checking this case. This seems like the appropriate place to warn about this rather than in brew shellenv.

It's unclear to me whether this check is incorrect or the information was omitted from the user's brew doctor output.

Let's change that instead of brew shellenv.

@gromgit
Copy link
Member Author

gromgit commented Aug 3, 2024

I actually see a case for doing both, as a kind of "layered defence":

  1. the glib patch targets all Homebrew users, but misses those whose XDG_DATA_DIRS is (unknowingly, for the majority of Linux users) set, so
  2. the brew shellenv PR makes everything Just Work, but misses those who subsequently reset XDG_DATA_DIRS to remove Homebrew's share dir, so
  3. the brew doctor check warns them about this, and catches any other situations we can't think of at this point

@gromgit gromgit mentioned this pull request Aug 3, 2024
7 tasks
@MikeMcQuaid
Copy link
Member

Passing on this in favour of #17947, thanks for both @gromgit!

2. the brew shellenv PR makes everything Just Work

As mentioned above: I specifically do not want:

  • to keep adding variables to brew shellenv based on very low numbers of issue reports
  • everything to "work" like this on macOS i.e. when we've had no macOS issue reports and when I have tried, and will continue to try, very hard to avoid XDG_* support creeping in on macOS
  • to add additional code when we find a broken brew doctor check that would have resolved the issue anyway

Thanks again for the PR, hopefully this explains my reasoning better.

@MikeMcQuaid MikeMcQuaid closed this Aug 5, 2024
@gromgit gromgit deleted the shellenv/xdg-data-dirs branch August 5, 2024 10:14
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.

Settings schema 'io.github.lxi-tools.lxi-gui' is not installed
3 participants