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

10.14: recognise Xcode 10.0 & add pkg-config files. #4275

Merged
merged 5 commits into from Jun 5, 2018

Conversation

Projects
None yet
4 participants
@DomT4
Copy link
Contributor

DomT4 commented Jun 4, 2018

  • 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 tests with your changes locally?

Still a bit of a WIP.

Fixes #4276

@mistydemeo

This comment has been minimized.

Copy link
Contributor

mistydemeo commented Jun 5, 2018

🤔 The clang I'm seeing from a CLT-only install doesn't return a valid value for DevelopmentTools.clang_version; regex isn't matching, so it returns Version::NULL. We'll need to update the regex to allow more than one digit in the major version:

diff --git a/Library/Homebrew/development_tools.rb b/Library/Homebrew/development_tools.rb
index 1f0d77d148..2ee5169476 100644
--- a/Library/Homebrew/development_tools.rb
+++ b/Library/Homebrew/development_tools.rb
@@ -63,7 +63,7 @@ class DevelopmentTools
     def clang_version
       @clang_version ||= begin
         if (path = locate("clang")) &&
-           build_version = `#{path} --version`[/(?:clang|LLVM) version (\d\.\d)/, 1]
+           build_version = `#{path} --version`[/(?:clang|LLVM) version (\d+\.\d)/, 1]
           Version.new build_version
         else
           Version::NULL
@@ -171,7 +173,8 @@ def detect_version_from_clang_version
when 81 then "8.3"
when 90 then "9.2"
when 91 then "9.3"
else "9.3"
when 10001 then "10.0"

This comment has been minimized.

@mistydemeo

mistydemeo Jun 5, 2018

Contributor

With the CLT, and the patch I mentioned earlier for clang_version, I'm seeing a value of 100 here, not 10001. What string are you getting for clang --version? Are you using Xcode, and what version?

This comment has been minimized.

@DomT4

DomT4 Jun 5, 2018

Contributor

I'm seeing a value of 100 here

That would be more like what I expected, to be honest! I assumed something was going on when 10001 popped out but I had to go grab some sleep so pushed it for review as-is in case someone else had a more sensible answer.

What string are you getting for clang --version?

Apple LLVM version 10.0.0 (clang-1000.10.25.5)
Target: x86_64-apple-darwin17.7.0
Thread model: posix
InstalledDir: /Applications/Xcode-beta.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin

Are you using Xcode, and what version?

Aye, 10.0.

Version: 1.1.29
Description: XSLT library version 2.
Requires: libxml-2.0
Libs: -L${libdir} -lxslt -lxml2 -lz -lpthread -licucore -lm

This comment has been minimized.

@ilovezfs

ilovezfs Jun 5, 2018

Contributor

looks like there are two spaces between xslt and xml2

This comment has been minimized.

@DomT4

DomT4 Jun 5, 2018

Contributor

Yeah, it's just a direct copy from the prior pkg-config set in this case since there's no version change as far as I can see. I can fix it if ya like but I think the double spacing goes back to at least 10.10 and possibly earlier.

This comment has been minimized.

@ilovezfs

ilovezfs Jun 5, 2018

Contributor

👀

This comment has been minimized.

@DomT4

DomT4 Jun 5, 2018

Contributor

Amusingly, the double spacing seems to be an upstream thing originally, and we've just kept it over the years.

Version: 0.8.17
Description: EXSLT Extension library
Requires: libxml-2.0
Libs: -L${libdir} -lexslt -lxslt -lxml2 -lz -lpthread -licucore -lm

This comment has been minimized.

@ilovezfs

ilovezfs Jun 5, 2018

Contributor

ditto

@DomT4

This comment has been minimized.

Copy link
Contributor

DomT4 commented Jun 5, 2018

The clang I'm seeing from a CLT-only install doesn't return a valid value for

Hey Misty! Thanks for the review here.

I'm running semi-blind on this at the moment because my developer program membership with Apple is about to expire and, well, I can't afford to renew it at the moment, so I haven't tried to spin up a box yet because I don't want to get stuck with a VM I'll only receive updates for for another week or so.

If you have more direct access & there's anything that'd be easier for you to handle please feel free to say and I'll happily surrender it to you; don't want to needlessly eat up your time working with me if you've a limited amount of it or anything.

DomT4 added some commits Jun 4, 2018

@DomT4 DomT4 force-pushed the DomT4:Mojave_DevTools_Updates branch from b4e983a to d1bd3fc Jun 5, 2018

@mistydemeo
Copy link
Contributor

mistydemeo left a comment

OK, great - this is working for me in my 10.14 VM! Thanks for updating.

@DomT4

This comment has been minimized.

Copy link
Contributor

DomT4 commented Jun 5, 2018

Nice! Thanks for the help 🙇.

@@ -171,7 +173,8 @@ def detect_version_from_clang_version
when 81 then "8.3"
when 90 then "9.2"
when 91 then "9.3"
else "9.3"
when 100 then "10.0"
else "10.0"

This comment has been minimized.

@ilovezfs

ilovezfs Jun 5, 2018

Contributor

what is the value for 9.4?

This comment has been minimized.

@DomT4

DomT4 Jun 5, 2018

Contributor

I completely forgot 9.4 had been released. Should probably update the when "10.13" then "9.3" as well.

It's still 91/9.1 though:

Apple LLVM version 9.1.0 (clang-902.0.39.2)
Target: x86_64-apple-darwin17.7.0
Thread model: posix
InstalledDir: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin

DomT4 added some commits Jun 5, 2018

@MikeMcQuaid
Copy link
Member

MikeMcQuaid left a comment

Thanks again @DomT4!

@MikeMcQuaid MikeMcQuaid merged commit 22e9fd7 into Homebrew:master Jun 5, 2018

1 of 3 checks passed

codecov/patch 10% of diff hit (target 69.59%)
Details
codecov/project 69.56% (-0.03%) compared to 7d25300
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@DomT4 DomT4 deleted the DomT4:Mojave_DevTools_Updates branch Jun 5, 2018

@DomT4

This comment has been minimized.

Copy link
Contributor

DomT4 commented Jun 5, 2018

Thanks everyone ❤️

@DomT4

This comment has been minimized.

Copy link
Contributor

DomT4 commented Jun 8, 2018

I haven't tried to spin up a box yet because I don't want to get stuck with a VM I'll only receive updates for for another week or so.

As a minor update I've set up a box locally now, so for as long as it keeps receiving updates & Apple doesn't rat me out to the update eligibility checker that my developer membership is about to expire I'll hopefully be of some use on 10.14 stuff.

@lock lock bot added the outdated label Jul 8, 2018

@lock lock bot locked as resolved and limited conversation to collaborators Jul 8, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.