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

Mojave: require CLT header package #4334

Merged
merged 3 commits into from Jun 14, 2018

Conversation

Projects
None yet
5 participants
@mistydemeo
Copy link
Contributor

mistydemeo commented Jun 12, 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?

As of Xcode 10, the CLT no longer installs the traditional Unix header layout by default. The CLT SDK is the only location in which headers are located. There is, however, a separate header package which can be installed which restores the traditional Unix-style headers. I've submitted two PRs to address this situation, with conflicting solutions - we should merge only one of the two.

I haven't added tests for the untested files yet, but I'll do that once we pick which of the two solutions we want to go for.

This PR attempts to retain the status quo by making the CLT header package mandatory. It makes the following changes:

  • Allows MacOS::CLT to detect whether the header package is relevant, whether it's installed, and whether it matches the installed CLT.
  • Prints header package information as a part of brew --config.
  • Adds a doctor check, and makes the header package mandatory for installing software in a CLT-only configuration.

See #4335 for a PR which uses the CLT SDKs instead.

@ilovezfs

This comment has been minimized.

Copy link
Contributor

ilovezfs commented Jun 13, 2018

👍

@commitay

This comment has been minimized.

Copy link
Contributor

commitay commented Jun 13, 2018

Would it be possible/practical to use /usr/sbin/installer to install this automatically?

@mistydemeo

This comment has been minimized.

Copy link
Contributor

mistydemeo commented Jun 13, 2018

Would it be possible/practical to use /usr/sbin/installer to install this automatically?

We can do that in the installer! I'll submit a PR for the installer if we decide to go with this branch. Once we're in Homebrew land though, we're no longer able to sudo on behalf of the user.

version
else
@header_version ||= MacOS.pkgutil_info(EXECUTABLE_PKG_ID)[/version: (.+)$/, 1]
@header_version.nil? ? ::Version::NULL : ::Version.new(@header_version)

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jun 13, 2018

Member

I'd make this an if block or early return as the ternary plus :: makes this hard to mentally parse.

This comment has been minimized.

@mistydemeo

mistydemeo Jun 14, 2018

Contributor

Made the identical change as in the other branch.

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Jun 13, 2018

Thanks for the PR and write-up @mistydemeo!

I've submitted two PRs to address this situation, with conflicting solutions - we should merge only one of the two.

Could merge this PR minus the doctor check immediately as that part of the diff is present in both PRs.

Would it be possible/practical to use /usr/sbin/installer to install this automatically?

We can do that in the installer! I'll submit a PR for the installer if we decide to go with this branch. Once we're in Homebrew land though, we're no longer able to sudo on behalf of the user.

This is my main concern with this approach; unless it's possible to auto-install this stuff with e.g. xcode-select --install or similar it's going to be pretty painful to require users to fiddle around in their browser to download and install the right package.

That said, I think given we've just had a single developer beta and this has a much slimmer diff than #4335 it's the right approach until we're closer to a Mojave release (or at least we've got some non-developer betas); I'd rather avoid monkeying with superenv if we can avoid it at all.

If people don't strongly disagree then I'd suggest: merge this PR pretty much as-is and then rebase, keep open and consider #4335 further down the line (removing the doctor check in the process) if we don't have a pleasant user experience to autoinstall these headers.

@ilovezfs

This comment has been minimized.

Copy link
Contributor

ilovezfs commented Jun 13, 2018

The pkg file itself gets installed along with the rest of the CLT, so it should be a question of printing "Please run ..."

  installer -pkg /Library/Developer/CommandLineTools/Packages/macOS_SDK_headers_for_macOS_10.14.pkg -target /

or wrap that with a brew install-clt-headers.

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Jun 13, 2018

The pkg file itself gets installed along with the rest of the CLT, so it should be a question of printing "Please run ..."

Cool. I'm definitely 👍 on this approach for now but I still think it's a step back having to get users to run Yet Another Command to have Homebrew work on 10.14.

@ilovezfs

This comment has been minimized.

Copy link
Contributor

ilovezfs commented Jun 13, 2018

I'm sure your sympathy will be noted by the complainers.

@mistydemeo mistydemeo force-pushed the mistydemeo:mojave_clt_headers branch from ba636b5 to e5212d7 Jun 14, 2018

@mistydemeo

This comment has been minimized.

Copy link
Contributor

mistydemeo commented Jun 14, 2018

I still think it's a step back having to get users to run Yet Another Command to have Homebrew work on 10.14.

Yep, I'm in agreement. I don't think we have much choice here though, unless we go with the #4335 approach.

@mistydemeo

This comment has been minimized.

Copy link
Contributor

mistydemeo commented Jun 14, 2018

That said, I think given we've just had a single developer beta and this has a much slimmer diff than #4335 it's the right approach until we're closer to a Mojave release (or at least we've got some non-developer betas); I'd rather avoid monkeying with superenv if we can avoid it at all.

That doesn't necessarily mean it won't change, but this is explicitly documented in Apple's release notes. It does appear to be the direction they intend to go with this release.

As a status-quo option that also ensures GCC and software using GCC works, I think this is the most straightforward solution.

@MikeMcQuaid MikeMcQuaid merged commit 9b21422 into Homebrew:master Jun 14, 2018

1 of 3 checks passed

codecov/patch 52% of diff hit (target 69.66%)
Details
codecov/project 69.59% (-0.07%) compared to e39b6f5
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@wafflebot wafflebot bot removed the in progress label Jun 14, 2018

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Jun 14, 2018

Agreed, thanks again @mistydemeo!

@mistydemeo mistydemeo deleted the mistydemeo:mojave_clt_headers branch Jun 14, 2018

@mistydemeo mistydemeo referenced this pull request Jun 14, 2018

Closed

Python: set flags for CLT on 10.14 #28733

4 of 4 tasks complete
@jeremyhu

This comment has been minimized.

Copy link

jeremyhu commented Jul 6, 2018

Please don't do this.

This package is going away. We will not be supporting it in the future. Please remove your dependency on it. It is being provided as an optional package as a crutch for you to use if you need to while working through issues (and filing radars if yo need support from Apple). Simply requiring your users to install that package is not a long-term solution.

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Jul 6, 2018

@jeremyhu Homebrew lead maintainer here 👋. Thanks for chiming in and providing that information. I'd really appreciate if we can get a more formal and private communication channel (e.g. private mailing list or Slack channel) with Apple for such things (as currently we have none) and to help us get access to the Mojave beta which we do not currently have access to. You can contact me at mike@mikemcquaid.com to discuss this further. Thanks!

@mistydemeo

This comment has been minimized.

Copy link
Contributor

mistydemeo commented Jul 6, 2018

This package is going away. We will not be supporting it in the future.

Is there information on this and any planned schedule? The Xcode 10 release notes don't say anything about this package being removed in the future, and when I asked about the headers in a radar I was directed to install this package.

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Jul 17, 2018

@jeremyhu Would love to get some answers on the above either publicly or privately or be directed to someone who could provide them.

@jeremyhu

This comment has been minimized.

Copy link

jeremyhu commented Jul 18, 2018

@MikeMcQuaid Sorry for missing this earlier. Let's followup in email.
@mistydemeo It should be in the release notes. I need to check. Can you point me to the radar you filed? I'd like to make sure messaging is clear in responses to future radars and want to followup on what was communicated.

@mistydemeo

This comment has been minimized.

Copy link
Contributor

mistydemeo commented Jul 18, 2018

@jeremyhu It was rdar://40833474.

Can you please include me on the email?

@jeremyhu

This comment has been minimized.

Copy link

jeremyhu commented Jul 18, 2018

The release note says,

Command Line Tools
The Command Line Tools package installs the macOS system headers inside the macOS SDK. Software that compiles with the installed tools will search for macOS headers at the standard include path:
/Applications/Xcode-
beta.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.14.sd
k/usr/include
For legacy software that looks for the macOS headers in the base system under /usr/include, please install the package file located at:
/Library/Developer/CommandLineTools/Packages/macOS_SDK_headers_for_macOS_10.14.pkg To make sure that you are using the intended version of the command line tools, run xcode-select -s
after installing.

which I admit is not explicitly clear on the deprecation.

@lock lock bot added the outdated label Aug 17, 2018

@lock lock bot locked as resolved and limited conversation to collaborators Aug 17, 2018

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