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.

Sign up for GitHub

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

Jump to bottom

llvm 11.0.0 #62798

Closed
wants to merge 5 commits into from
Closed

llvm 11.0.0 #62798

wants to merge 5 commits into from

Conversation

kubkon
Copy link
Contributor

@kubkon kubkon commented Oct 13, 2020

  • 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 the automerge-skip `brew pr-automerge` will skip this pull request label Oct 13, 2020
@fxcoudert fxcoudert mentioned this pull request Oct 13, 2020
Closed
@fxcoudert
Copy link
Member

Can we add a short test that runs llvm-config and checks that it doesn't output things like -llibxml2.tbd? see #62480

@fxcoudert
Copy link
Member

ccls and castxml each need a revision bump (a separate commit for each, please)

@kubkon
Copy link
Contributor Author

kubkon commented Oct 13, 2020

Can we add a short test that runs llvm-config and checks that it doesn't output things like -llibxml2.tbd? see #62480

Lemme see what I can figure out.

@kubkon kubkon force-pushed the llvm-11 branch 2 times, most recently from 47015ed to 5f4adeb Compare October 13, 2020 15:29
@kubkon
Copy link
Contributor Author

kubkon commented Oct 13, 2020 •
edited
Loading

ccls and castxml each need a revision bump (a separate commit for each, please)

@fxcoudert done in revised 127f06b. Lemme know if it makes sense though since I'm not very proficient in Ruby.

@jedisct1
Copy link
Contributor

I still get the -llibxml2.tbd issue :(

@kubkon
Copy link
Contributor Author

kubkon commented Oct 13, 2020 •
edited
Loading

I still get the -llibxml2.tbd issue :(

With LLVM 11? Huh, interesting cause I don't. We'll see if the test case passes in the CI.

@jedisct1
Copy link
Contributor

Yep, with LLVM 11, just compiled using your Formula. This is using Xcode 12.2 beta on Big Sur, though.

@kubkon
Copy link
Contributor Author

kubkon commented Oct 13, 2020

Yep, with LLVM 11, just compiled using your Formula. This is using Xcode 12.2 beta on Big Sur, though.

If the CI passes fine here, I reckon it'd be a good idea to inform upstream LLVM of the issue on Big Sur.

@kubkon kubkon mentioned this pull request Oct 13, 2020
Closed
@bcardiff
Copy link
Contributor

Regarding LLVM 11.0.0. They are aware about it AFAIK

Also note that the changes are not likely to be backported to 11 [..]. There should be enough time for 11.0.1, though.
https://reviews.llvm.org/D87590#2273743

@fxcoudert
Copy link
Member

Regarding LLVM 11.0.0. They are aware about it AFAIK

If there is a patch, we can consider it. But we need to know that it works, that upstream is considering it.

@kubkon
Copy link
Contributor Author

kubkon commented Oct 13, 2020

@jedisct1 @fxcoudert so it seems that my machine must be special, or perhaps I have something (mis)configured in a very special way; the -llibxml2.tbd regression is still there. What do you think the next steps should be? Should we merge this commenting out the test case and wait for the patch from upstream (after confirming with LLVM maintainers that it will happen first)?

@jedisct1
Copy link
Contributor

The patch is pretty short and simple.

If we can confirm that it works, maybe we should include it, no matter if this is the exact code that will eventually get merged upstream.

And we can always update the patch later if needed.

kubkon
kubkon commented Oct 13, 2020
View reviewed changes
Formula/llvm.rb Outdated Show resolved Hide resolved
@SMillerDev
Copy link
Member

We should really take the upstream patch unless it's impossible and/or upstream is unwilling to fix it.

fxcoudert
fxcoudert reviewed Oct 13, 2020
View reviewed changes
Formula/llvm.rb Outdated Show resolved Hide resolved
@jayschwa jayschwa mentioned this pull request Oct 13, 2020
Closed
@kubkon kubkon force-pushed the llvm-11 branch 4 times, most recently from a23a5c4 to 8cd1c1c Compare October 13, 2020 22:42
@timmyjose
Copy link

I think special big thanks is due to the Homebrew team for working with the Zig folks so quickly and efficiently! It's quite heartening to see it in fact.

tschoonj
tschoonj reviewed Oct 17, 2020
View reviewed changes
Copy link
Contributor

@tschoonj tschoonj left a comment

Choose a reason for hiding this comment

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

Why was libcxxabi added to the runtimes?

@kubkon
Copy link
Contributor Author

kubkon commented Oct 17, 2020

Why was libcxxabi added to the runtimes?

What do you mean? The previous formula already did that however only for HEAD with a note to check when llvm-11 is released.

@tschoonj
Copy link
Contributor

Why was libcxxabi added to the runtimes?

What do you mean? The previous formula already did that however only for HEAD with a note to check when llvm-11 is released.

Sorry, missed the note...

jonchang
jonchang reviewed Oct 18, 2020
View reviewed changes
Formula/llvm.rb Outdated Show resolved Hide resolved
Formula/llvm.rb Show resolved Hide resolved
@kubkon
Copy link
Contributor Author

kubkon commented Oct 18, 2020

Does anyone know if it is possible to apply a series of patches after all resource blocks are downloaded and extracted into the tree? This way I could leave the patches as they are while reverting back the formula to downloading all llvm components separately which should fix the CI.

@jonchang
Copy link
Contributor

If you need to patch a resource I believe the patch block works inside the resource block too. Does that help?

@kubkon
Copy link
Contributor Author

kubkon commented Oct 18, 2020

If you need to patch a resource I believe the patch block works inside the resource block too. Does that help?

Unfortunately, it won't. The issue here is that we need to have all components in place before we apply the patches. In other words, first, we need to download llvm-src, clang-src, etc., extract them into one common dir, and then we can apply the patches since the patches are messy and touch upon multiple subprojects in one diff.

@kubkon
Copy link
Contributor Author

kubkon commented Oct 18, 2020

OK, nvm, the hashes of diffs changed after I added full_index=1. Correcting now.

@kubkon
Use only upstream patches; use single download for llvm-project
a67e922 
This commit changes the formula to download only a single archive
`llvm-project` (instead of multiple components), and applies unchanged
upstream patches to fix the behaviour of `llvm-config --system-libs`.

Signed-off-by: Jakub Konka <[email protected]>
@fxcoudert
Copy link
Member

downloading all llvm components separately which should fix the CI

What is the problem with CI and llvm-project?

@kubkon
Copy link
Contributor Author

kubkon commented Oct 18, 2020

downloading all llvm components separately which should fix the CI

What is the problem with CI and llvm-project?

False alarm :-) I thought we hit the GitHub rate limit but it was my oversight with the hashes of the patches. It should be fixed now. Sorry for confusion!

@kubkon kubkon mentioned this pull request Oct 18, 2020
Merged
@kubkon
Copy link
Contributor Author

kubkon commented Oct 18, 2020

Yay, everything passed! I reckon we're good to go if you agree! :-)

@jedisct1
Copy link
Contributor

Woo-ooh!

@fxcoudert
Copy link
Member

Many thanks for the collective work on this one!

@BrewTestBot
Copy link
Member

:shipit: @fxcoudert has triggered a merge.

@kubkon kubkon deleted the llvm-11 branch October 18, 2020 17:13
@lmurray
Copy link

lmurray commented Oct 19, 2020

I noticed that llvm@10 will be dropped with this PR as is

Correct, unless we have other formulas that strictly depend on LLVM 10 (and won't build with 11), which does not look like it's the case. We do not intend to carry every version of the software, especially when it is not maintained anymore.

I would like to chime in here to point out that the removal of llvm@10 has temporarily broken some users' development environments such as my own. Due to LLVM versions not being fully compatible with each other I pin my environments to specific versions of dependencies and then upgrade them when I have the available time to debug issues that occur due to the upgrade. Right now my environments are pinned by llvm@10 but it's no longer available, preventing me from installing a new environment on another system, or even rolling back if an upgrade to llvm@11 fails.

Currently llvm@9, llvm@8, llvm@7, and llvm@6 are already still available which means the removal of llvm@10 also goes against precedence.

I would like to propose that older versions of LLVM, and potentially other packages used by developers, are kept around at least for a few months so that the above issues are not encountered by users.

@lmurray
Copy link

lmurray commented Oct 19, 2020

Although I do admit that maybe I shouldn't be using Homebrew for this particular use case in the first place as it may be out of Homebrew's scope. Users that version pin could just use the official builds directly instead of going through Homebrew. This is what I do on other platforms anyway.

@fxcoudert
Copy link
Member

Our criteria for versioned formulas are set out here: https://docs.brew.sh/Versions
If you believe LLVM 10 meets these criteria, feel free to open a pull request.

@lmurray
Copy link

lmurray commented Oct 19, 2020

Thank you for your response. The linked document clearly states that my use case of version pinning is not in Homebrew's scope which means there is no need to add LLVM 10 back.

@SMillerDev
Copy link
Member

Currently llvm@9, llvm@8, llvm@7, and llvm@6 are already still available which means the removal of llvm@10 also goes against precedence.

Rules change constantly based on new insights. Past results do not guarantee an outcome for the future in the case of Homebrew formulae.

@chohner chohner mentioned this pull request Nov 29, 2020
Closed
@will will mentioned this pull request Dec 31, 2020
Closed
@carlocab carlocab mentioned this pull request Apr 15, 2021
Closed
@asfimport asfimport mentioned this pull request Oct 24, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bayandin bayandin bayandin left review comments

@jonchang jonchang jonchang left review comments

@jedisct1 jedisct1 jedisct1 left review comments

@tschoonj tschoonj tschoonj left review comments

@fxcoudert fxcoudert fxcoudert approved these changes

Assignees
No one assigned
Labels
automerge-skip `brew pr-automerge` will skip this pull request CI-requeued PR has been re-added to the queue
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.