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

carlocab · 2022-08-07T15:47:09Z

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?

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.

`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.

BrewTestBot · 2022-08-07T15:47:26Z

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

Companion to Homebrew/brew#13664.

carlocab · 2022-08-07T15:50:32Z

pkg-config rebuild: Homebrew/homebrew-core#107528

Bo98 · 2022-08-07T16:01:45Z

Library/Homebrew/brew.sh

  else
-    HOMEBREW_OS_VERSION="macOS ${HOMEBREW_MACOS_VERSION}"
+    HOMEBREW_MACOS_VERSION_NUMBER="${HOMEBREW_MACOS_VERSION%.*}"


The name HOMEBREW_MACOS_VERSION_NUMBER feels too similar to HOMEBREW_MACOS_VERSION_NUMERIC.

This does slightly change how analytics work but I don't know if we actually have full access to patch versions there anyway or not (and if such data is useful or not).

I can also just skip defining HOMEBREW_MACOS_VERSION_NUMBER and strip out the "macOS " from HOMEBREW_OS_VERSION. Would that be better?

As for patch information, I think we just end up throwing that away. We discard patch information in our analytics reporting, for example. I haven't checked if dropping the patch version breaks the aggregation.

I honestly see no reason to strip patch information from HOMEBREW_OS_VERSION so it really could just be a constant "macOS ${HOMEBREW_MACOS_VERSION}" for that.

I'm not entirely sure why the original patch stripping was implemented. I believe it was when from when our analytics page had separate entries for each macOS full version but that is no longer the case as we merge them over on the homebrew-formula-analytics side.

Yeah it was analytics (8af4895) so it should indeed no longer be an issue since Homebrew/homebrew-formula-analytics@3be1121 (the original commit was incomplete anyway).

Feel free to simplify and remove the special casing.

I've removed the special casing from generating HOMEBREW_OS_VERSION, but we still need it to find the right pkgconfig directory.

I've also changed HOMEBREW_MACOS_VERSION_NUMBER.

Rylan12

From what I can tell, changing HOMEBREW_OS_VERSION will change the OS_VERSION constant. This is used at least in DevelopmentTools.build_system_info. Changing the would affect Tab#build_on for those macOS versions. Not sure that's actually used anywhere.

It also affects Utils::Analytics.os_arch_prefix_ci. Changing this affects the event label that's sent to the analytics, but it doesn't seem like we actually query that label anywhere so I kinda think it's fine as is unless you just want to be safe.

Library/Homebrew/brew.sh

We now have better handling of macOS versions in our analytics, so this is no longer needed. We still need to strip the version conditional on the macOS version in order to identify the right `pkgconfig` directory. This partially reverts 8af4895.

Library/Homebrew/brew.sh

Co-authored-by: Bo Anderson <mail@boanderson.me>

Bo98

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

MikeMcQuaid · 2022-08-08T13:08:39Z

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?

Bo98 · 2022-08-08T13:27:29Z

@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 formula: ensure that kegs with version_scheme == -1 are outdated #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.

MikeMcQuaid · 2022-08-08T13:50:08Z

But this by chance happens to be the same problem as formula: ensure that kegs with version_scheme == -1 are outdated #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.

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

BrewTestBot · 2022-08-09T00:07:11Z

Review period ended.

carlocab · 2022-08-11T07:18:08Z

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 · 2022-08-11T07:23:57Z

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.

MikeMcQuaid · 2022-08-11T08:08:43Z

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.

carlocab · 2022-08-15T11:00:47Z

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 · 2022-08-15T13:05:10Z

Library/Homebrew/cmd/update-report.rb

+  def symlink_current_pkgconfig_directory
+    return if OS.linux?
+
+    pkgconfig_dir = HOMEBREW_LIBRARY/"Homebrew/os/mac/pkgconfig"
+    FileUtils.rm_f pkgconfig_dir/"current"
+    pkgconfig_dir.install_symlink MacOS.version => "current"
+  end


This actually feels a bit more out of place for me here, since brew.sh seems more reasonable for setup code that makes sure brew is working properly.

This seems like a reasonable place to me. I'd suggest symlink_current_pkgconfig_directory be pulled into a helper function, though, and it also be called in install.rb or formula_installer.rb because you cannot rely on all users running brew update. It feels like it also warrants a brew doctor if missing.

I'd suggest symlink_current_pkgconfig_directory be pulled into a helper function, though, and it also be called in install.rb or formula_installer.rb because you cannot rely on all users running brew update. It feels like it also warrants a brew doctor if missing.

Yes, agreed.

Could actually even be a good candidate for one of the diagnostic pre-install mandatory checks that just refuses to continue if it's broken, runs as part of brew doctor and we can tell people to run brew update or whatever to fix.

Added symlink creation to install.rb and a diagnostic.

Could actually even be a good candidate for one of the diagnostic pre-install mandatory checks that just refuses to continue if it's broken

I contemplated doing this, but am ambivalent. If @Bo98 thinks it's a good idea then I'm happy to do it though.

MikeMcQuaid · 2022-08-15T13:40:45Z

Library/Homebrew/brew.sh

  # Don't change this from Mac OS X to match what macOS itself does in Safari on 10.12
  HOMEBREW_OS_USER_AGENT_VERSION="Mac OS X ${HOMEBREW_MACOS_VERSION}"

  # Intentionally set this variable by exploding another.
  # shellcheck disable=SC2086,SC2183
  printf -v HOMEBREW_MACOS_VERSION_NUMERIC "%02d%02d%02d" ${HOMEBREW_MACOS_VERSION//./ }

-  # Don't include minor versions for Big Sur and later.


Why this change? Feels like we need to audit uses of HOMEBREW_OS_VERSION to check this won't have unintended effects.

This change grew out of the discussion here: #13664 (comment)

I can revert it, since it's not actually related to what I want to do here anymore (it was tangentially related to what I was trying to do in brew.sh earlier).

Ah, gotcha. I'm fine with it if you're happy with the various usages being fine 👍🏻

My understanding was this was done to fix our analytics not being able to handle it, but that's fixed now.

In any case, the previous code didn't strip enough on Big Sur, which still comes with patch versions these days (e.g. 11.5.1) but Monterey does not (e.g. 12.5), and that seemed to work ok (i.e. the rest of the code dealt with macOS 11.5 just fine). This suggests to me that keeping the entire version number doesn't hurt.

MikeMcQuaid · 2022-08-15T13:42:04Z

Library/Homebrew/cmd/update-report.rb

+  def symlink_current_pkgconfig_directory
+    return if OS.linux?
+
+    pkgconfig_dir = HOMEBREW_LIBRARY/"Homebrew/os/mac/pkgconfig"
+    FileUtils.rm_f pkgconfig_dir/"current"
+    pkgconfig_dir.install_symlink MacOS.version => "current"
+  end


This seems like a reasonable place to me. I'd suggest symlink_current_pkgconfig_directory be pulled into a helper function, though, and it also be called in install.rb or formula_installer.rb because you cannot rely on all users running brew update. It feels like it also warrants a brew doctor if missing.

We can't always rely on users doing `brew update`, so let's do this for `brew install`, `brew reinstall`, and `brew upgrade` too.

carlocab · 2022-08-15T15:11:45Z

Library/Homebrew/os/linux.rb

@@ -63,6 +63,8 @@ def sdk_path
      nil
    end

+    def symlink_current_pkgconfig_directory; end


@MikeMcQuaid I know you don't like these MacOS compat methods, so I can move this up into the OS module if preferred. It kinda makes sense for it to be here for me, though.

MikeMcQuaid

Sorry for more comments, almost there 🤞🏻

MikeMcQuaid · 2022-08-15T15:26:23Z

Library/Homebrew/install.rb

@@ -15,6 +15,7 @@ module Install
    module_function

    def perform_preinstall_checks(all_fatal: false, cc: nil)
+      MacOS.symlink_current_pkgconfig_directory


Could maybe just make this a attempt_pkgconfig_directory_symlink below attempt_directory_creation?

Would that not restrict it to being run only inside install.rb, instead of also during brew update?

Yes. I actively think it should not be run by brew update 😁. Could we try not doing it in brew update and ship a follow-up PR to add it there (with critical and self-merge without review if necessary) if we actually get any user reports that the brew update addition will fix?

I think the opposite: it's weird, and probably wrong, to not do this during brew update.

The symlink is part of the internal state of brew; it even lives inside HOMEBREW_LIBRARY. A brew update should not leave that state out of sync.

The symlink is part of the internal state of brew; it even lives inside HOMEBREW_LIBRARY. A brew update should not leave that state out of sync.

What actually breaks if this is out of sync outside of installations?

MikeMcQuaid · 2022-08-15T15:26:51Z

Library/Homebrew/os/mac.rb

+
+    # Avoid mandatory reinstalls of `pkg-config` upon OS upgrade.
+    sig { void }
+    def symlink_current_pkgconfig_directory


Yeh, I'm afraid this does feel weird to me in macOS, it doesn't seem to have anything to do with the OS other than the fact it's a no-op on Linux.

I don't think it's that weird -- it's specific to macOS because that's where we want to provide .pc files to help find system libraries. We don't currently have a reliable set of system libraries that we need to ship .pc files for on Linux.

@carlocab Yeh, but it's not a function of the OS itself, it's just OS-specific code. It should be a function living somewhere else that's just a no-op on non-MacOS.

MikeMcQuaid · 2022-08-15T15:28:58Z

Library/Homebrew/extend/os/mac/diagnostic.rb

+            rm -rf #{current_symlink} && \\
+              ln -sfn #{MacOS.version} #{current_symlink}


Should this just say "run brew whatever" if that cleans it up?

MikeMcQuaid · 2022-08-15T15:30:57Z

Library/Homebrew/extend/os/mac/diagnostic.rb

@@ -488,6 +488,20 @@ def check_broken_sdks
          #{installation_instructions}
        EOS
      end
+
+      def check_pkgconfig_current_symlink


I'd suggest this is added to e.g. fatal_preinstall_checks

MikeMcQuaid · 2022-08-23T10:47:04Z

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

carlocab · 2022-08-23T11:43:34Z

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

MikeMcQuaid · 2022-08-23T11:57:24Z

@carlocab Not that soon! A week or two?

carlocab · 2022-08-23T12:02:29Z

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

carlocab · 2022-08-31T13:17:07Z

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

there's a more general approach that handles the necessary symlinking
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?)

MikeMcQuaid · 2022-08-31T14:47:59Z

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?

github-actions · 2022-09-22T14:50:22Z

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.

BrewTestBot added the waiting for feedback Merging is blocked until sufficient time has passed for review label Aug 7, 2022

carlocab added a commit to carlocab/homebrew-core that referenced this pull request Aug 7, 2022

pkg-config: point to current symlink

455b76f

Companion to Homebrew/brew#13664.

carlocab mentioned this pull request Aug 7, 2022

pkg-config: point to current symlink Homebrew/homebrew-core#107528

Closed

6 tasks

carlocab requested a review from Bo98 August 7, 2022 15:51

Bo98 reviewed Aug 7, 2022

View reviewed changes

Rylan12 reviewed Aug 7, 2022

View reviewed changes

Library/Homebrew/brew.sh Outdated Show resolved Hide resolved

Bo98 reviewed Aug 8, 2022

View reviewed changes

Library/Homebrew/brew.sh Outdated Show resolved Hide resolved

brew.sh: match Big Sur more accurately

76b3eeb

Co-authored-by: Bo Anderson <mail@boanderson.me>

Bo98 approved these changes Aug 8, 2022

View reviewed changes

BrewTestBot removed the waiting for feedback Merging is blocked until sufficient time has passed for review label Aug 9, 2022

BrewTestBot approved these changes Aug 9, 2022

View reviewed changes

Move current symlink creation into update-report

9b7b156

carlocab commented Aug 15, 2022

View reviewed changes

MikeMcQuaid reviewed Aug 15, 2022

View reviewed changes

carlocab added 2 commits August 15, 2022 22:31

install: update pkgconfig/current symlink

eb1d378

We can't always rely on users doing `brew update`, so let's do this for `brew install`, `brew reinstall`, and `brew upgrade` too.

mac/diagnostic: add check for pkgconfig/current directory

15340da

carlocab commented Aug 15, 2022

View reviewed changes

MikeMcQuaid reviewed Aug 15, 2022

View reviewed changes

github-actions bot added the stale No recent activity label Sep 22, 2022

github-actions bot closed this Sep 30, 2022

github-actions bot added the outdated PR was locked due to age label Oct 31, 2022

github-actions bot locked as resolved and limited conversation to collaborators Oct 31, 2022

		rm -rf #{current_symlink} && \\
		ln -sfn #{MacOS.version} #{current_symlink}

brew.sh: create current symlink in pkgconfig directory #13664

brew.sh: create current symlink in pkgconfig directory #13664

Conversation

carlocab commented Aug 7, 2022

BrewTestBot commented Aug 7, 2022

carlocab commented Aug 7, 2022

Choose a reason for hiding this comment

carlocab Aug 7, 2022 • edited

Choose a reason for hiding this comment

Bo98 Aug 7, 2022 • edited

Choose a reason for hiding this comment

Bo98 Aug 8, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rylan12 left a comment

Choose a reason for hiding this comment

Bo98 left a comment

Choose a reason for hiding this comment

MikeMcQuaid commented Aug 8, 2022

Bo98 commented Aug 8, 2022 • edited

MikeMcQuaid commented Aug 8, 2022

BrewTestBot commented Aug 9, 2022

carlocab commented Aug 11, 2022 • edited

carlocab commented Aug 11, 2022

MikeMcQuaid commented Aug 11, 2022

carlocab commented Aug 15, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikeMcQuaid left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikeMcQuaid commented Aug 23, 2022

carlocab commented Aug 23, 2022

MikeMcQuaid commented Aug 23, 2022

carlocab commented Aug 23, 2022

carlocab commented Aug 31, 2022

MikeMcQuaid commented Aug 31, 2022

github-actions bot commented Sep 22, 2022

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

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

carlocab Aug 7, 2022 •

edited

Bo98 Aug 7, 2022 •

edited

Bo98 Aug 8, 2022 •

edited

Bo98 commented Aug 8, 2022 •

edited

carlocab commented Aug 11, 2022 •

edited