brew icon indicating copy to clipboard operation
brew copied to clipboard

brew.sh: create `current` symlink in `pkgconfig` directory

Open carlocab opened this issue 1 year ago • 17 comments

  • [x] Have you followed the guidelines in our Contributing document?
  • [x] Have you checked to ensure there aren't other open Pull Requests for the same change?
  • [x] 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.
  • [x] 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?

pkg-config currently hardcodes the OS version it was built on in its pc path in order to find the .pc files shipped with Homebrew. This requires reinstallation of pkg-config with OS upgrades, which is something we currently do not handle.

We can avoid this by automatically creating a current symlink that points to the right directory whenever brew.sh runs. This will require a rebuild of pkg-config that points it to the current directory.

In order to simplify identifying the right directory to symlink to, I've dropped the patch version from HOMEBREW_OS_VERSION on Catalina and earlier. I don't think this should break anything, but I can try a different approach if this will cause problems.

carlocab avatar Aug 07 '22 15:08 carlocab

Review period will end on 2022-08-09 at 00:00:00 UTC.

BrewTestBot avatar Aug 07 '22 15:08 BrewTestBot

pkg-config rebuild: https://github.com/Homebrew/homebrew-core/pull/107528

carlocab avatar Aug 07 '22 15:08 carlocab

This requires reinstallation of pkg-config with OS upgrades, which is something we currently do not handle.

@Bo98 had done some work in this direction. What's the latest here @Bo98?

No idea if it's the right place to put this code or not, but functionally this looks good to me.

Yeh, I'd rather this was something that was done inside Ruby-land rather than Bash here (given no Bash commands rely on this). Feels like perhaps in e.g. formula_installer.rb?

MikeMcQuaid avatar Aug 08 '22 13:08 MikeMcQuaid

@Bo98 had done some work in this direction. What's the latest here @Bo98?

The data is now all there (compare the built_on OS in the tab to the running OS). The two main things left was:

  • A DSL so that we could mark which formulae should have this behaviour.
  • Make brew upgrade actually work with formula that are marked as outdated like this, since it currently only supports pkg_version changing updates. My initial thought is this could be worked around by appending the macOS version to the formula version number. But this by chance happens to be the same problem as #13661 so maybe something more generic can be done instead.

Even if we end up not needing this for pkg-config, there are other formulae which would find this functionality useful.

Bo98 avatar Aug 08 '22 13:08 Bo98

Even if we end up not needing this for pkg-config, there are other formulae which would find this functionality useful.

Agreed, feels like a general solution might be nice here.

MikeMcQuaid avatar Aug 08 '22 13:08 MikeMcQuaid

Review period ended.

BrewTestBot avatar Aug 09 '22 00:08 BrewTestBot

Agreed, feels like a general solution might be nice here.

I agree with aiming for a general solution where we reinstall formulae with OS upgrades when needed.

However, the need to reinstall pkg-config is because we hardcode the OS version in its search path. A solution where we don't do that probably won't be general, but is better than reinstalling to fix the hard-coded OS version.

carlocab avatar Aug 11 '22 07:08 carlocab

Yeh, I'd rather this was something that was done inside Ruby-land rather than Bash here (given no Bash commands rely on this). Feels like perhaps in e.g. formula_installer.rb?

We can move it into Ruby-land, but I don't think formula_installer is the right place.

The symlink can become stale due to things happening outside of brew, so waiting for another brew install before updating it means that users could be stuck with a stale symlink for longer than necessary.

My idea was that we want to make sure this symlink is updated with every brew invocation, and the closest I could come to that was putting it here.

carlocab avatar Aug 11 '22 07:08 carlocab

We can move it into Ruby-land, but I don't think formula_installer is the right place.

I'm fine with it being in a different place.

My idea was that we want to make sure this symlink is updated with every brew invocation, and the closest I could come to that was putting it here.

Do we have other things that e.g. make brew list make changes on disk? That feels a bit weird to me.

To me only brew install/upgrade/reinstall/uninstall/autoremove/cleanup are what I'd expect to write changes on disk like this. brew doctor could warn if it's missing.

MikeMcQuaid avatar Aug 11 '22 08:08 MikeMcQuaid

I think of the symlink as part of the internal state of brew, it just so happens to live on-disk. We also write to the disk when modifying .git/config (e.g. when running a dev command, or on any invocation of brew where homebrew.analyticsmessage is true but homebrew.analyticsuuid is unset), and I view this as similar to those.

But I don't mind moving this into brew update. I'm going to do that.

carlocab avatar Aug 15 '22 11:08 carlocab

@carlocab would be good to get this in for 3.6.0 if you get the chance 🙇🏻

MikeMcQuaid avatar Aug 23 '22 10:08 MikeMcQuaid

How soon did you want to get 3.6.0 out? Will try to get this ready with sufficient time for review before then.

carlocab avatar Aug 23 '22 11:08 carlocab

@carlocab Not that soon! A week or two?

MikeMcQuaid avatar Aug 23 '22 11:08 MikeMcQuaid

Ok, will work on this tomorrow. Feel free to nudge if you don't hear anything by Thursday.

carlocab avatar Aug 23 '22 12:08 carlocab

Apologies for the delay here; I was mulling over how specific this solution really is to pkg-config, and it seems we have a bunch of symlink handling elsewhere too (e.g. Linux-preferred GCC, but we want to get rid of those).

I wonder if

  1. there's a more general approach that handles the necessary symlinking
  2. creating symlinks might be how we might want to avoid the reinstall-at-OS-upgrade problem in other formulae too (e.g. a symlink to the correct SDK inside HOMEBREW_PREFIX?)

carlocab avatar Aug 31 '22 13:08 carlocab

2. creating symlinks might be how we might want to avoid the reinstall-at-OS-upgrade problem in other formulae too (e.g. a symlink to the correct SDK inside HOMEBREW_PREFIX?)

TBH I think we should just handle the reinstall-at-OS-upgrade next time brew install/brew upgrade or maybe even brew update is run after an OS upgrade?

MikeMcQuaid avatar Aug 31 '22 14:08 MikeMcQuaid

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

github-actions[bot] avatar Sep 22 '22 14:09 github-actions[bot]