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

Improved Command Lines Tool update instructions #11396

Merged
merged 3 commits into from May 18, 2021
Merged

Improved Command Lines Tool update instructions #11396

merged 3 commits into from May 18, 2021

Conversation

gattilorenz
Copy link

Added link to Apple's page with latest version of XCode for each macOS release to the message that suggest downloading latest version.

Test fails on my machine but it's only a string in the XCode file, and Ruby's syntax checker doesn't complain about it - the failing test is
rspec ./test/cmd/--version_spec.rb:7 # brew --version prints the Homebrew's version
so completely unrelated.

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

Lorenzo Gatti added 2 commits May 17, 2021 17:11
#{Formatter.url("https://developer.apple.com/download/more/")}.
#{Formatter.url("https://developer.apple.com/download/more/")}
after checking the latest available version for your system on:
#{Formatter.url("https://developer.apple.com/support/xcode/#minimum-requirements")}.
Copy link
Member

Choose a reason for hiding this comment

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

We know these versions so we could perhaps just tell people the versions instead?

Copy link
Member

Choose a reason for hiding this comment

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

see latest_version or minimum_version

Copy link
Author

Choose a reason for hiding this comment

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

Are they the same as the XCode versions, or can the version numbers be different across the two? I assumed they are disconnected, since apparently I had the latest XCode but not the latest CLT version installed.

I only see the latest human-readable version as XCode.latest_version since CLT.version seems to use different version numbers (and there's a comment about how "different ways of installing the CLTs lead to different version numbers" in the code)

Copy link
Member

Choose a reason for hiding this comment

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

The versions differ, right. @Bo98 any thoughts here (as you've been thinking about this recently)

Copy link
Author

Choose a reason for hiding this comment

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

Actually, I'm not sure anymore whether there is a problem in practice.

The goal of the message is to let the user know which version is the latest supported CLT version on their OS, so it only has to match the version that is found on Apple's website, right? Which seems consistent with the latest XCode's version for the same system.
I'm not sure why my tools were outdated while my XCode was not, and how I had installed them last time. However, the latest CLT on Apple's website is still 11.3.1 - same as latest XCode for Mojave - and that's what I downloaded and installed to fix the problem today.

So maybe it does make sense to indicate XCode's version to the user, not just to check for it to determine whether the tools are up-to-date?

Copy link
Member

Choose a reason for hiding this comment

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

So maybe it does make sense to indicate XCode's version to the user, not just to check for it to determine whether the tools are up-to-date?

Yes, this makes sense to me 👍🏻 MacOS::Xcode.latest_version will get this version for you.

Copy link
Author

Choose a reason for hiding this comment

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

alright, if the wording is ok (not a native speaker) this should be it

@MikeMcQuaid
Copy link
Member

Thanks so much for your first contribution! Without people like you submitting PRs we couldn't run this project. You rock, @gattilorenz!

Copy link
Member

@Rylan12 Rylan12 left a comment

Choose a reason for hiding this comment

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

Thanks again, @gattilorenz! (I've triggered a rerun of the tests and this will auto-merge when those are complete)

@Rylan12
Copy link
Member

Rylan12 commented May 18, 2021

Actually, something's really wrong with the CI runs here so I'm going to close and reopen and hopefully that will resolve it 😅

@Rylan12 Rylan12 closed this May 18, 2021
auto-merge was automatically disabled May 18, 2021 21:10

Pull request was closed

@Rylan12 Rylan12 reopened this May 18, 2021
@Rylan12 Rylan12 enabled auto-merge May 18, 2021 21:10
@Rylan12 Rylan12 merged commit d375d04 into Homebrew:master May 18, 2021
@github-actions github-actions bot added the outdated PR was locked due to age label Jun 18, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 18, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants