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

llvm 10.0.0 #52087

Closed
wants to merge 9 commits into from
Closed

llvm 10.0.0 #52087

wants to merge 9 commits into from

Conversation

smorimoto
Copy link
Contributor

@smorimoto smorimoto commented Mar 24, 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>)?

@smorimoto
Copy link
Contributor Author

The test succeeds, but there is a problem. I don't know what approach to take in brew to avoid this problem, so I would like to hear the opinion.

λ brew audit --strict llvm
llvm:
  * python modules have explicit framework links
    These python extension modules were linked directly to a Python
    framework binary. They should be linked with -undefined dynamic_lookup
    instead of -lpython or -framework Python.
      /usr/local/opt/llvm/lib/python2.7/site-packages/lldb/_lldb.so

@BrewTestBot
Copy link
Member

  • New formulae in homebrew/core should not have a bottle do block

@SMillerDev
Copy link
Member

Please leave LLVM@9 out for now. You could make a new pull request for it if needed.

@Bo98
Copy link
Member

Bo98 commented Mar 25, 2020

Can you reduce the diff on llvm? You moved blocks around for no reason and actually in ways that make the audit fail:

==> brew audit llvm --online
==> FAILED
Error: 3 problems in 1 formula detected
llvm:
  * C: 19: col 3: `stable` (line 19) should be put before `pour_bottle?` (line 6)
  * C: 69: col 3: `head` (line 69) should be put before `keg_only` (line 11)

Also oclgrind will need a revision bump.

@smorimoto smorimoto force-pushed the llvm-10.0.0 branch 2 times, most recently from 9ca1cc3 to 694cdb7 Compare March 25, 2020 03:49
@timmyjose
Copy link

Please expedite this! :-)

@SMillerDev
Copy link
Member

Please consider hosting a copy in a personal tap if you want this done fast, which is very easy to do: https://docs.brew.sh/How-to-Create-and-Maintain-a-Tap

If you want it done well you'll have to wait for maintainers to have time to review the changes and for CI to test it.

@Bo98
Copy link
Member

Bo98 commented Mar 25, 2020

oclgrind might just need a revision bump rather than llvm@9, unless you tried it out locally and it didn't work.

@timmyjose
Copy link

@SMillerDev It was a friendly suggestion. I very well know that it takes time to review and test - no need to get snarky.

Copy link
Member

@Bo98 Bo98 left a comment

Choose a reason for hiding this comment

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

  • Remove Aliases/llvm@9. You don't need an alias to something of the same name.
Error: Formula duplicating alias: /usr/local/Homebrew/Library/Taps/homebrew/homebrew-core/Aliases/llvm@9
  • It looks like oclgrind does in fact need llvm@9, but still keep the revision bump as well since it will be needed to ensure users have the right dependency.

  • The test of llvm@9 fails. Here's what I think will fix it:

Formula/llvm@9.rb Outdated Show resolved Hide resolved
@smorimoto
Copy link
Contributor Author

Thank you for the review. I applied that.

@bcardiff
Copy link
Contributor

Thanks @imbsky for this PR!

The current Formula/crystal.rb will also need depends_on "llvm@9". In the next version it will have support for llvm 10. Should that be fixed in this pr as Formula/oclgrid.cr or in a separate one?

@Bo98
Copy link
Member

Bo98 commented Mar 26, 2020

Probably this one. Can the test be improved to pick up those cases better? It seems like the current one never fails if the LLVM version is incompatible. What action breaks under LLVM 10?

@bcardiff
Copy link
Contributor

I will try to improve them for some smoke testing. Because right now the issue appears when building from source probably or running the compiler specs that are to heavy for the CI.

@Bo98
Copy link
Member

Bo98 commented Mar 26, 2020

If it only fails to build from source, I could potentially add llvm to the list of formulae which test building dependents from source, if that doesn't end up being too costly time-wise.

@iMichka
Copy link
Member

iMichka commented Mar 27, 2020

llvm@9:
  * Versioned formulae in homebrew/core should use `keg_only :versioned_formula`
  * Formulae should not have a `HEAD` spec

@imbsky can you please run brew audit llvm@9 --online --new-formula llvm@9 and brew audit --online llvm before pushing any changes? Running this build blocks our CI for 5 to 6 hours each time, so it would be great if audit errors are fixed before pushing. Thanks.

@smorimoto
Copy link
Contributor Author

I'm sorry, I made some unnecessary commits, so please cancel them manually if necessary. I will try to test it in advance from next time. The only remaining problem is the following. Does anyone have a solution?

λ brew audit --online llvm
llvm:
  * python modules have explicit framework links
    These python extension modules were linked directly to a Python
    framework binary. They should be linked with -undefined dynamic_lookup
    instead of -lpython or -framework Python.
      /usr/local/opt/llvm/lib/python2.7/site-packages/lldb/_lldb.so
Error: 1 problem in 1 formula detected

BSKY added 2 commits March 28, 2020 23:21
Since LLVM 10.0.0, LLDB_DISABLE_PYTHON has been renamed to LLDB_ENABLE_PYTHON.
@smorimoto
Copy link
Contributor Author

The last commit should pass the test.

@smorimoto
Copy link
Contributor Author

It seems that the build was aborted for some reason.

@Bo98
Copy link
Member

Bo98 commented Mar 28, 2020

It's already requeued. There were some CI issues earlier.

@smorimoto
Copy link
Contributor Author

I wasn't used to Jenkins so I didn't know how to check it. Thank you!

@smorimoto
Copy link
Contributor Author

All tests have passed!

@smorimoto smorimoto requested a review from Bo98 March 30, 2020 05:23
@smorimoto
Copy link
Contributor Author

@SMillerDev @Bo98 @iMichka Could you check this?

@Bo98
Copy link
Member

Bo98 commented Mar 30, 2020

I think this is good. I checked the bottle earlier and nothing seems to be missing. I'm going to check crystal in a moment and then I'll merge this.

@smorimoto
Copy link
Contributor Author

@Bo98 Thank you!

@smorimoto
Copy link
Contributor Author

smorimoto commented Mar 30, 2020

When I built the Crystal compiler locally with brew's LLVM package (10.0.0), there seemed to be no problems.

@bcardiff
Copy link
Contributor

Lucky us! It is usually a problem. If something arises, I will fix it later, we should be near to release the next version with llvm 10 support though.

@Bo98 Bo98 closed this in e28ad8b Mar 30, 2020
@Bo98
Copy link
Member

Bo98 commented Mar 30, 2020

Thanks!

@smorimoto smorimoto deleted the llvm-10.0.0 branch March 30, 2020 22:32
@smorimoto
Copy link
Contributor Author

@Bo98 Thank you, too!

@timmyjose
Copy link

Thank you to @imbsky and all the folks who worked on this - I had built llvm+lld+clang 10 on my own in the interim, but it took around 40GB (!) on my local machine, so this is a very welcome release. Cheers!

@lock lock bot added the outdated PR was locked due to age label May 6, 2020
@lock lock bot locked as resolved and limited conversation to collaborators May 6, 2020
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

7 participants