Merge pull request #4335 from mistydemeo/mojave_clt_sdk

Mojave: use the CLT SDK where necessary

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

@@ -20,7 +20,6 @@ module Homebrew
           check_xcode_minimum_version
           check_clt_minimum_version
           check_if_xcode_needs_clt_installed
-          check_if_clt_needs_headers_installed
         ].freeze
       end
 
@@ -138,17 +137,6 @@ module Homebrew
         EOS
       end
 
-      def check_if_clt_needs_headers_installed
-        return unless MacOS::CLT.separate_header_package?
-        return if MacOS::CLT.headers_installed?
-
-        <<~EOS
-          The Command Line Tools header package must be installed on #{MacOS.version.pretty_name}.
-          The installer is located at:
-            #{MacOS::CLT::HEADER_PKG_PATH.sub(":macos_version", MacOS.version)}
-        EOS
-      end
-
       def check_for_other_package_managers
         ponk = MacOS.macports_or_fink
         return if ponk.empty?

Library/Homebrew/extend/os/mac/extend/ENV/std.rb

@@ -27,8 +27,8 @@ module Stdenv
 
     append_path "ACLOCAL_PATH", "#{MacOS::X11.share}/aclocal"
 
-    if MacOS::XQuartz.provided_by_apple? && !MacOS::CLT.installed?
+    if MacOS::XQuartz.provided_by_apple? && MacOS.sdk_path_if_needed
-      append_path "CMAKE_PREFIX_PATH", "#{MacOS.sdk_path}/usr/X11"
+      append_path "CMAKE_PREFIX_PATH", "#{MacOS.sdk_path_if_needed}/usr/X11"
     end
 
     append "CFLAGS", "-I#{MacOS::X11.include}" unless MacOS::CLT.installed?
@@ -93,7 +93,7 @@ module Stdenv
     delete("CPATH")
     remove "LDFLAGS", "-L#{HOMEBREW_PREFIX}/lib"
 
-    return unless (sdk = MacOS.sdk_path(version)) && !MacOS::CLT.installed?
+    return unless (sdk = MacOS.sdk_path_if_needed(version))
     delete("SDKROOT")
     remove_from_cflags "-isysroot #{sdk}"
     remove "CPPFLAGS", "-isysroot #{sdk}"
@@ -115,7 +115,7 @@ module Stdenv
     self["CPATH"] = "#{HOMEBREW_PREFIX}/include"
     prepend "LDFLAGS", "-L#{HOMEBREW_PREFIX}/lib"
 
-    return unless (sdk = MacOS.sdk_path(version)) && !MacOS::CLT.installed?
+    return unless (sdk = MacOS.sdk_path_if_needed(version))
     # Extra setup to support Xcode 4.3+ without CLT.
     self["SDKROOT"] = sdk
     # Tell clang/gcc where system include's are:
@@ -132,7 +132,7 @@ module Stdenv
 
   # Some configure scripts won't find libxml2 without help
   def libxml2
-    if MacOS::CLT.installed?
+    if !MacOS.sdk_path_if_needed
       append "CPPFLAGS", "-I/usr/include/libxml2"
     else
       # Use the includes form the sdk

Library/Homebrew/extend/os/mac/extend/ENV/super.rb

@@ -58,7 +58,7 @@ module Superenv
   def homebrew_extra_library_paths
     paths = []
     if compiler == :llvm_clang
-      if MacOS::CLT.installed?
+      if !MacOS.sdk_path_if_needed
         paths << "/usr/lib"
       else
         paths << "#{MacOS.sdk_path}/usr/lib"
@@ -102,7 +102,7 @@ module Superenv
   end
 
   def effective_sysroot
-    MacOS.sdk_path.to_s if MacOS::Xcode.without_clt?
+    MacOS.sdk_path_if_needed&.to_s
   end
 
   def set_x11_env_if_installed
@@ -113,7 +113,6 @@ module Superenv
   def setup_build_environment(formula = nil)
     generic_setup_build_environment(formula)
     self["HOMEBREW_SDKROOT"] = effective_sysroot
-    self["SDKROOT"] = MacOS.sdk_path if MacOS::Xcode.without_clt?
 
     # Filter out symbols known not to be defined since GNU Autotools can't
     # reliably figure this out with Xcode 8 and above.

Library/Homebrew/os/mac.rb

@@ -85,18 +85,13 @@ module OS
     #      specifically been requested according to the rules above.
 
     def sdk(v = nil)
-      @locator ||= SDKLocator.new
+      @locator ||= if Xcode.installed?
-      begin
+        XcodeSDKLocator.new
-        sdk = if v.nil?
-          (Xcode.version.to_i >= 7) ? @locator.latest_sdk : @locator.sdk_for(version)
       else
-          @locator.sdk_for v
+        CLTSDKLocator.new
       end
-      rescue SDKLocator::NoSDKError
+
-        sdk = @locator.latest_sdk
+      @locator.sdk_if_applicable(v)
-      end
-      # Only return an SDK older than the OS version if it was specifically requested
-      sdk if v || (!sdk.nil? && sdk.version >= version)
     end
 
     # Returns the path to an SDK or nil, following the rules set by #sdk.
@@ -105,6 +100,23 @@ module OS
       s&.path
     end
 
+    def sdk_path_if_needed(v = nil)
+      # Prefer Xcode SDK when both Xcode and the CLT are installed.
+      # Expected results:
+      # 1. On Xcode-only systems, return the Xcode SDK.
+      # 2. On Xcode-and-CLT systems where headers are provided by the system, return nil.
+      # 3. On CLT-only systems with no CLT SDK, return nil.
+      # 4. On CLT-only systems with a CLT SDK, where headers are provided by the system, return nil.
+      # 5. On CLT-only systems with a CLT SDK, where headers are not provided by the system, return the CLT SDK.
+
+      # If there's no CLT SDK, return early
+      return if MacOS::CLT.installed? && !MacOS::CLT.provides_sdk?
+      # If the CLT is installed and provides headers, return early
+      return if MacOS::CLT.installed? && !MacOS::CLT.separate_header_package?
+
+      sdk_path(v)
+    end
+
     # See these issues for some history:
     # https://github.com/Homebrew/legacy-homebrew/issues/13
     # https://github.com/Homebrew/legacy-homebrew/issues/41

Library/Homebrew/os/mac/sdk.rb

@@ -11,7 +11,7 @@ module OS
       end
     end
 
-    class SDKLocator
+    class BaseSDKLocator
       class NoSDKError < StandardError; end
 
       def sdk_for(v)
@@ -28,17 +28,33 @@ module OS
         SDK.new v, path
       end
 
+      def sdk_if_applicable(v = nil)
+        sdk = begin
+          if v.nil?
+            (source_version.to_i >= 7) ? latest_sdk : sdk_for(OS::Mac.version)
+          else
+            sdk_for v
+          end
+        rescue BaseSDKLocator::NoSDKError
+          latest_sdk
+        end
+        # Only return an SDK older than the OS version if it was specifically requested
+        return unless v || (!sdk.nil? && sdk.version >= OS::Mac.version)
+        sdk
+      end
+
       private
 
+      def source_version
+        OS::Mac::Version::NULL
+      end
+
+      def sdk_prefix
+        ""
+      end
+
       def sdk_paths
         @sdk_paths ||= begin
-          # Xcode.prefix is pretty smart, so let's look inside to find the sdk
-          sdk_prefix = "#{Xcode.prefix}/Platforms/MacOSX.platform/Developer/SDKs"
-          # Xcode < 4.3 style
-          sdk_prefix = "/Developer/SDKs" unless File.directory? sdk_prefix
-          # Finally query Xcode itself (this is slow, so check it last)
-          sdk_prefix = File.join(Utils.popen_read(DevelopmentTools.locate("xcrun"), "--show-sdk-platform-path").chomp, "Developer", "SDKs") unless File.directory? sdk_prefix
-
           # Bail out if there is no SDK prefix at all
           if !File.directory? sdk_prefix
             {}
@@ -55,5 +71,52 @@ module OS
         end
       end
     end
+
+    class XcodeSDKLocator < BaseSDKLocator
+      private
+
+      def source_version
+        OS::Mac::Xcode.version
+      end
+
+      def sdk_prefix
+        @sdk_prefix ||= begin
+          # Xcode.prefix is pretty smart, so let's look inside to find the sdk
+          sdk_prefix = "#{Xcode.prefix}/Platforms/MacOSX.platform/Developer/SDKs"
+          # Xcode < 4.3 style
+          sdk_prefix = "/Developer/SDKs" unless File.directory? sdk_prefix
+          # Finally query Xcode itself (this is slow, so check it last)
+          sdk_platform_path = Utils.popen_read(DevelopmentTools.locate("xcrun"), "--show-sdk-platform-path").chomp
+          sdk_prefix = File.join(sdk_platform_path, "Developer", "SDKs") unless File.directory? sdk_prefix
+
+          sdk_prefix
+        end
+      end
+    end
+
+    class CLTSDKLocator < BaseSDKLocator
+      private
+
+      def source_version
+        OS::Mac::CLT.version
+      end
+
+      # While CLT SDKs existed prior to Xcode 10, those packages also
+      # installed a traditional Unix-style header layout and we prefer
+      # using that
+      # As of Xcode 10, the Unix-style headers are installed via a
+      # separate package, so we can't rely on their being present.
+      # This will only look up SDKs on Xcode 10 or newer, and still
+      # return nil SDKs for Xcode 9 and older.
+      def sdk_prefix
+        @sdk_prefix ||= begin
+          if !CLT.provides_sdk?
+            ""
+          else
+            "#{CLT::PKG_PATH}/SDKs"
+          end
+        end
+      end
+    end
   end
 end

Library/Homebrew/os/mac/xcode.rb

@@ -96,6 +96,16 @@ module OS
         !prefix.nil?
       end
 
+      def sdk(v = nil)
+        @locator ||= XcodeSDKLocator.new
+
+        @locator.sdk_if_applicable(v)
+      end
+
+      def sdk_path(v = nil)
+        sdk(v)&.path
+      end
+
       def update_instructions
         if MacOS.version >= "10.9" && !OS::Mac.prerelease?
           <<~EOS
@@ -211,7 +221,11 @@ module OS
       end
 
       def separate_header_package?
-        MacOS.version >= :mojave
+        version >= "10"
+      end
+
+      def provides_sdk?
+        version >= "8"
       end
 
       def headers_installed?
@@ -222,6 +236,16 @@ module OS
         end
       end
 
+      def sdk(v = nil)
+        @locator ||= CLTSDKLocator.new
+
+        @locator.sdk_if_applicable(v)
+      end
+
+      def sdk_path(v = nil)
+        sdk(v)&.path
+      end
+
       def update_instructions
         if MacOS.version >= "10.9"
           <<~EOS

Library/Homebrew/test/os/mac/diagnostic_spec.rb

@@ -32,19 +32,6 @@ describe Homebrew::Diagnostic::Checks do
       .to match("Xcode alone is not sufficient on El Capitan")
   end
 
-  specify "#check_if_clt_needs_headers_installed" do
-    allow(MacOS).to receive(:version).and_return(OS::Mac::Version.new("10.14"))
-    allow(MacOS::CLT).to receive(:installed?).and_return(true)
-    allow(MacOS::CLT).to receive(:headers_installed?).and_return(false)
-
-    expect(subject.check_if_clt_needs_headers_installed)
-      .to match("The Command Line Tools header package must be installed on Mojave.")
-
-    allow(MacOS).to receive(:version).and_return(OS::Mac::Version.new("10.13"))
-    expect(subject.check_if_clt_needs_headers_installed)
-      .to be_nil
-  end
-
   specify "#check_homebrew_prefix" do
     # the integration tests are run in a special prefix
     expect(subject.check_homebrew_prefix)

Library/Homebrew/test/os/mac_spec.rb

@@ -19,4 +19,44 @@ describe OS::Mac do
       expect { Locale.parse(subject.language) }.not_to raise_error
     end
   end
+
+  describe "::sdk_path_if_needed" do
+    it "calls sdk_path on Xcode-only systems" do
+      allow(OS::Mac::Xcode).to receive(:installed?) { true }
+      allow(OS::Mac::CLT).to receive(:installed?) { false }
+      expect(OS::Mac).to receive(:sdk_path)
+      OS::Mac.sdk_path_if_needed
+    end
+
+    it "does not call sdk_path on Xcode-and-CLT systems with system headers" do
+      allow(OS::Mac::Xcode).to receive(:installed?) { true }
+      allow(OS::Mac::CLT).to receive(:installed?) { true }
+      allow(OS::Mac::CLT).to receive(:separate_header_package?) { false }
+      expect(OS::Mac).not_to receive(:sdk_path)
+      OS::Mac.sdk_path_if_needed
+    end
+
+    it "does not call sdk_path on CLT-only systems with no CLT SDK" do
+      allow(OS::Mac::Xcode).to receive(:installed?) { false }
+      allow(OS::Mac::CLT).to receive(:installed?) { true }
+      expect(OS::Mac).not_to receive(:sdk_path)
+      OS::Mac.sdk_path_if_needed
+    end
+
+    it "does not call sdk_path on CLT-only systems with a CLT SDK if the system provides headers" do
+      allow(OS::Mac::Xcode).to receive(:installed?) { false }
+      allow(OS::Mac::CLT).to receive(:installed?) { true }
+      allow(OS::Mac::CLT).to receive(:separate_header_package?) { false }
+      expect(OS::Mac).not_to receive(:sdk_path)
+      OS::Mac.sdk_path_if_needed
+    end
+
+    it "calls sdk_path on CLT-only systems with a CLT SDK if the system does not provide headers" do
+      allow(OS::Mac::Xcode).to receive(:installed?) { false }
+      allow(OS::Mac::CLT).to receive(:installed?) { true }
+      allow(OS::Mac::CLT).to receive(:separate_header_package?) { true }
+      expect(OS::Mac).to receive(:sdk_path)
+      OS::Mac.sdk_path_if_needed
+    end
+  end
 end