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

Provides a config option to ignore warnings about Xcode being outdated #18509

Closed
wants to merge 2 commits into from

Conversation

ctaintor
Copy link
Contributor

@ctaintor ctaintor commented Oct 5, 2024

  • 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?

I'm adding this since in our organization, there are parts of the organization which are slower about updating xcode versions. In our case we are using an older version of Xcode 15 – so, still on an Xcode from this year, just not 15.4. This is because we rely on Simulator visual screenshots and these can sometimes change very slightly between Xcode versions. We want to silence the warning for the users who are updating Xcode in a controlled manner.

Copy link
Member

@carlocab carlocab left a comment

Choose a reason for hiding this comment

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

A bit on the fence about this, but I won't block it if other maintainers are fine with it.

Library/Homebrew/extend/os/mac/diagnostic.rb Outdated Show resolved Hide resolved
Comment on lines +400 to +403
HOMEBREW_NO_WARN_OUTDATED_XCODE: {
description: "If set, Homebrew will not warn about using outdated (but still compatible) Xcode versions.",
boolean: true,
},
Copy link
Member

Choose a reason for hiding this comment

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

I'm inclined not to document this at all:

Suggested change
HOMEBREW_NO_WARN_OUTDATED_XCODE: {
description: "If set, Homebrew will not warn about using outdated (but still compatible) Xcode versions.",
boolean: true,
},

in which case you should ignore my other suggestion about Homebrew::EnvConfig (since that relies on the env variable being in env_config.rb).

@carlocab carlocab requested a review from a team October 5, 2024 13:31
@p-linnane
Copy link
Member

I'm 👎 on this. Silencing a legitimate warning because an enterprise is slow to update doesn't make sense to me.

@ctaintor
Copy link
Contributor Author

ctaintor commented Oct 5, 2024

Right above this suggested change, there is code to ignore these warnings in github actions. The reason that is wanted in the github actions environment is similar to the reason for this PR – there are environments where you cannot update Xcode the day it comes out and you'd prefer not to have a warning print.

It feels like removing the linked code and instead setting HOMEBREW_NO_WARN_OUTDATED_XCODE=1 for the CI environment is another way to reason about this change. In that way, it would have clear value to the Homebrew project while also enabling users of Homebrew to ignore the warning when their project work means that they can't upgrade Xcode immediately.

@SMillerDev
Copy link
Member

I don't think we should silence these, they are legitimate warnings and companies being slow to update doesn't stop that. Just like we don't hide the warnings that macOS versions are outdated.

@ctaintor
Copy link
Contributor Author

ctaintor commented Oct 5, 2024

Just for clarity – I am talking about using a version of Xcode 15 which is not the latest (the latest version of Xcode is 15.4 and was released in May) but was still released less than 1 year ago. I am just asking that the same control of warning messages used for the CI is exposed to normal users. Also – any debugging or support requested from the brew community always includes the ´brew doctor´output, so this warning would be printed there.

I went and looked at how older macOS versions are handled by brew. I think there are some differences from this:

  • the oldest version of macOS supported by brew is based on the major release, not a minor release
  • the oldest release of macOS supported by brew is 4 years old
  • a brew user can configure the oldest version supported by setting HOMEBREW_MACOS_OLDEST_SUPPORTED. It can be moved back to support even older versions of macOS.

@Bo98
Copy link
Member

Bo98 commented Oct 5, 2024

Worth noting that this warning already should not appear if you are installing from bottles and only when installing from source. I'm assuming you're using a third-party tap that doesn't use bottles?

Does your xcode-select -p pointing to the CLT? We could maybe silence this warning on the condition that the CLT is used (and the formula in question doesn't use depends_on :xcode).

@ctaintor
Copy link
Contributor Author

ctaintor commented Oct 5, 2024

Thanks for pointing that out – I was unaware. Yes, we are using our own (internal) tap – most are built from source, mostly because the infra to build/update bottles is a challenge to set up. (None depend_on xcode). They are node, python or go CLIs.

Does your ´xcode-select -p´ pointing to the CLT?

I'm not entirely sure what you mean – it points to the /Applications/Xcode-15.2.0.app/Contents/Developer

@gromgit
Copy link
Member

gromgit commented Oct 6, 2024

Does your ´xcode-select -p´ pointing to the CLT?

I'm not entirely sure what you mean – it points to the /Applications/Xcode-15.2.0.app/Contents/Developer

Then it's not:

% xcode-select -p
/Library/Developer/CommandLineTools

@MikeMcQuaid
Copy link
Member

The reason that is wanted in the github actions environment is similar to the reason for this PR – there are environments where you cannot update Xcode the day it comes out and you'd prefer not to have a warning print.

Actually the only reason we silence them there is because it's our internal CI system.

Sorry, passing on this.

@MikeMcQuaid MikeMcQuaid closed this Oct 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants