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

diagnostic: make using outdated dev tools fatal #1097

Merged
merged 1 commit into from Sep 23, 2016
Merged

diagnostic: make using outdated dev tools fatal #1097

merged 1 commit into from Sep 23, 2016

Conversation

DomT4
Copy link
Member

@DomT4 DomT4 commented Sep 22, 2016

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

A smarter form of 559cea7. Travis users can't force Travis to update 10.11 to Xcode 8, so this was murdering builds left, right & centre.

Fixes #1096 whilst still retaining the point of the original commit. Also offers developers an opt-out so if we need to test something on 10.11 with Xcode 7.x we can, etc.

A smarter form of 559cea7.
Travis users can't force Travis to update 10.11 to Xcode 8, so this was murdering
builds left, right & centre.

Fixes #1096 whilst still retaining the
point of the original commit. Also offers developers an opt-out so if we need
to test something on 10.11 with Xcode 7.x we can, etc.
@MikeMcQuaid
Copy link
Member

Seems fine. I wonder(ed) if we wanted to enforce only certain versions of Xcode/CLT in this that we know to be problematic e.g. Xcode 7 on Sierra.

@MikeMcQuaid MikeMcQuaid merged commit 81e325c into Homebrew:master Sep 23, 2016
@DomT4 DomT4 deleted the software_dev_is_hard branch September 23, 2016 17:56
@DomT4
Copy link
Member Author

DomT4 commented Sep 23, 2016

Thanks for the merge! Do you want a PR with further tweaks here or are you happy?

@MikeMcQuaid
Copy link
Member

Think it's fine as-is. We can iterate if people 😭

@DomT4
Copy link
Member Author

DomT4 commented Sep 23, 2016

Cool 👍

@chdiza
Copy link
Contributor

chdiza commented Sep 23, 2016

My apologies if I misunderstand the nature of this commit. This comment is about the user-friendliness of the commit, not the motivation or the implementation.

I think we should provide clear instructions how a user who is temporarily stuck with a newly-obsolete Xcode can get out of jail.

I'm thinking of cases where an Xcode bump comes out but a user hasn't had the chance to download it yet: either because she's on a slow connection, or even just because Xcode is an enormous download and takes a while even on broadband, or because she needs to perform careful backups and such before installing a potentially disruptive Xcode bump.

I don't like the idea that such a user---who intends to update to the latest version very soon---would be (or believe herself to be) frozen out of using homebrew until then. There may even be a critical update to something that she needs to brew right away, but can't because she is still waiting until she can install the latest Xcode.

I think we can keep old Xcodes being fatal so long as (a) the commit that kills a given Xcode is always tagged, and (b) in the fatality warning message we either provide clear and visible instructions on how she can use git to revert to the brew tag immediately prior to the one that killed her Xcode, or inform her that setting HOMEBREW_DEVELOPER will get her out of jail, or both.

@MikeMcQuaid
Copy link
Member

I don't like the idea that such a user---who intends to update to the latest version very soon---would be (or believe herself to be) frozen out of using homebrew until then. There may even be a critical update to something that she needs to brew right away, but can't because she is still waiting until she can install the latest Xcode.

I think so long as we wait until a bit after a major release it's reasonable for us to force this through. Xcode 7 on Sierra, for example, is known to be extremely broken. Perhaps we could restrict to more finely grained situations like that but I don't really ever want to allow people to install (even if they really have to) in that situation.

@zmwangx
Copy link
Contributor

zmwangx commented Sep 23, 2016

As this currently stands Xcode 7 on El Capitan is also fatal, which could be controversial.

@DomT4
Copy link
Member Author

DomT4 commented Sep 23, 2016

There may even be a critical update to something that she needs to brew right away, but can't because she is still waiting until she can install the latest Xcode.

If we've already moved over to the new Xcode we can't make any guarantees the old Xcode will work, and indeed, as homebrew/core has punchily shown over the last few days a lot of the time it won't, and because we've just upgraded ourselves there's very little we can do to help you debug in that situation.

I'm open to further tweaking, but I think fairly clearly we need to be harder on this sort of thing than we were.

@zmwangx
Copy link
Contributor

zmwangx commented Sep 23, 2016

I'd say nagging is better than failing out right on less problematic combinations (Xcode 7 on El Capitan). You don't really need latest everything to pour bottles, for instance.

@DomT4
Copy link
Member Author

DomT4 commented Sep 23, 2016

I guess I'm wary that going forwards the conditional could get a little more wild, for example right now:

if MacOS.version == :sierra && MacOS::Xcode.installed? && MacOS::Xcode.version < "8.0"

Is semi-doable, but come a year's time that might become something like:

if MacOS.version == :sierra && MacOS::Xcode.installed? && MacOS::Xcode.version < "8.0" || MacOS.version == :cupertino && MacOS::Xcode.installed? && MacOS::Xcode.version < "9.0"

etc, that's kind of getting out of hand.

@zmwangx
Copy link
Contributor

zmwangx commented Sep 23, 2016

But the bug reports...

@MikeMcQuaid
Copy link
Member

It feels like the Right Solution may be just to have a minimum Xcode version set somewhere per-OS. Make it data in a hash rather than a conditional. We store the latest but we could also store a minimum; we warn if not latest and fail if not minimum. Should be easy to write and enforce.

@chdiza
Copy link
Contributor

chdiza commented Sep 23, 2016

As this currently stands Xcode 7 on El Capitan is also fatal, which could be controversial.

This is exactly the sort of case I had in mind. (Plus, AFAICT right now, Xcode 8 on ElCap is more likely to cause a problem than Xcode 7 on ElCap.)

@chdiza
Copy link
Contributor

chdiza commented Sep 23, 2016

If we've already moved over to the new Xcode we can't make any guarantees the old Xcode will work, and indeed, as homebrew/core has punchily shown over the last few days a lot of the time it won't,

We can't make any guarantees, but: probably it will still work. Especially if the user reverts to the tag prior to the killing of the Xcode.

@chdiza
Copy link
Contributor

chdiza commented Sep 23, 2016

I think fairly clearly we need to be harder on this sort of thing than we were.

Agreed 😄, I just think we need to offer users a way out if they're in a jam at the moment.

@chdiza
Copy link
Contributor

chdiza commented Sep 23, 2016

We store the latest but we could also store a minimum; we warn if not latest and fail if not minimum. Should be easy to write and enforce.

I like this idea.

Separate from whatever the plan is, I think it's a good idea to make sure that Xcode-killing commits are tagged (or that the commit just prior to the killer is tagged).

@DomT4
Copy link
Member Author

DomT4 commented Sep 23, 2016

@chdiza If you want it done ASAP, you may want to work up a Pull Request. Otherwise improving this is on my list, but not right at the top.

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

None yet

4 participants