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

Warn when using Xcode 8 without CLT on 10.11 #965

Merged
merged 1 commit into from
Sep 16, 2016

Conversation

mistydemeo
Copy link
Contributor

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

Xcode 8 on 10.11 has more issues than the usual mismatched SDK problems, and we're going to get a decent degree of breakage. I think it's important that brew doctor tell people to install the CLT in this case.

It may be worth rethinking whether we want to support the Xcode-only-without-OS-specific-SDK usecase going forward. We've put a good amount of work into supporting Xcode-only over the years, but we're getting increasing amounts of pain for our efforts. I'd be happy to turn this PR into a blanket warn for that usecase, instead of something that only covers Xcode 8 on 10.11.

cc @DomT4

@BrewTestBot BrewTestBot added the in progress Maintainers are working on this label Sep 15, 2016
@mistydemeo mistydemeo force-pushed the warn_xcode_8_without_clt branch from b9aa004 to 235d03a Compare September 15, 2016 08:00
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 to me 👍

@@ -90,6 +91,18 @@ def check_clt_up_to_date
EOS
end

def check_xcode_8_without_clt_on_el_cap
Copy link
Member

Choose a reason for hiding this comment

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

Can we go for el_capitan here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

return unless MacOS.version == :el_capitan && MacOS::Xcode.version >= "8"

<<-EOS.undent
You have Xcode 8 installed without the CLT; this causes certain builds to fail.
Copy link
Member

Choose a reason for hiding this comment

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

Worth spelling out this is El Capitan-only to avoid scaring Sierra people?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The warning only prints on El Cap; are you worried about copy/pasted versions of it on the web being misinterpreted?

Copy link
Member

Choose a reason for hiding this comment

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

Yeh, or people reading this message when they have a mixed environment and thinking it applies everywhere.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed it's probably best to explicitly mention El Cap. It'll only be a matter of time until someone sticks the message on stackoverflow or something without much context 🙈.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated!

<<-EOS.undent
You have Xcode 8 installed without the CLT; this causes certain builds to fail.
Please install the CLT via:
xcode-select --install
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe a good idea to recommend sudo here as xcode-select will only abort unless you give it elevated privs.

If we're using a vanilla non-sudo recommendation everywhere else though feel free to ignore me. No point introducing an inconsistency.

Copy link
Contributor Author

@mistydemeo mistydemeo Sep 15, 2016

Choose a reason for hiding this comment

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

It's totally inconsistent now I look at it - we have messages using both with and without. 😰 I'll update this now, and make a PR to update other messages for consistency later.

@mistydemeo mistydemeo force-pushed the warn_xcode_8_without_clt branch from 20da8f5 to 842881b Compare September 16, 2016 03:45
@mistydemeo mistydemeo force-pushed the warn_xcode_8_without_clt branch from 842881b to 8a0861f Compare September 16, 2016 03:47
@mistydemeo mistydemeo merged commit 67cb634 into Homebrew:master Sep 16, 2016
@mistydemeo mistydemeo deleted the warn_xcode_8_without_clt branch September 16, 2016 06:22
@BrewTestBot BrewTestBot removed the in progress Maintainers are working on this label Sep 16, 2016
@MikeMcQuaid
Copy link
Member

@mistydemeo Is this still a problem? I'm wondering now the storm is over if it's worth reverting this. Alternatively, we wait until the 8.2 CLT on 10.11 (which is coming) and do it then.

CC @ilovezfs who talked about this with me.

@Homebrew Homebrew locked and limited conversation to collaborators May 3, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants