Skip to content

llvm@11: patch support for macOS 11.3+ SDK changes#76030

Closed
Bo98 wants to merge 2 commits intoHomebrew:masterfrom
Bo98:llvm-11-sdk11.3
Closed

llvm@11: patch support for macOS 11.3+ SDK changes#76030
Bo98 wants to merge 2 commits intoHomebrew:masterfrom
Bo98:llvm-11-sdk11.3

Conversation

@Bo98
Copy link
Copy Markdown
Member

@Bo98 Bo98 commented Apr 27, 2021

  • Have you followed the guidelines for contributing?
  • Have you checked that there aren't other open pull requests for the same formula update/change?
  • Have you built your formula locally with brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
  • Is your test running fine brew test <formula>, where <formula> is the name of the formula you're submitting?
  • Does your build pass brew audit --strict <formula> (after doing brew install <formula>)?

@BrewTestBot BrewTestBot added legacy Relates to a versioned @ formula python Python use is a significant feature of the PR or issue labels Apr 27, 2021
Comment on lines +60 to +61
(libexec/"lib").install_symlink llvm.opt_lib/"clang"
(libexec/"include").install_symlink llvm.opt_include/"c++"
Copy link
Copy Markdown
Member

@carlocab carlocab Apr 27, 2021

Choose a reason for hiding this comment

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

This doesn't help -- llvm.lib == llvm.opt_lib outside the llvm formula. We used to just copy the headers, but then eventually it still needed revision bumps anyway with minor llvm version bumps, so I figured going with symlinks worked better anyway.

https://rubydoc.brew.sh/Formula.html#prefix-instance_method

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Ah yes.

I'll undo, but it will be interesting to see if a revision bump helps here at least.

Copy link
Copy Markdown
Member Author

@Bo98 Bo98 Apr 27, 2021

Choose a reason for hiding this comment

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

Answer: it does.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Seems like we were symlinking to the Cellar? c++ -> ../../../../llvm@11/11.1.0/include/c++

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yea, not surprising. Revision bumps always help IWYU. Though I have to say I'm confused as to why it would now need one...

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Because if we are symlinking to the Cellar then the path is now 11.1.0_1 rather than 11.1.0.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ah, yes. We were symlinking to the cellar. I do remember noting that install_symlink seems a bit eager about resolving symlinks that you pass to it.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@carlocab carlocab mentioned this pull request Apr 27, 2021
@BrewTestBot
Copy link
Copy Markdown
Contributor

🤖 A scheduled task has triggered a merge.

@github-actions github-actions bot added the outdated PR was locked due to age label Jun 9, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 9, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

legacy Relates to a versioned @ formula outdated PR was locked due to age python Python use is a significant feature of the PR or issue

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants