Repository navigation
Conversation
|
Some previous discussion regarding those flags: |
|
As suggested by a colleague who knows far more about python than I do, I ran |
Most of the relevant discussion in this link seems to be about the fact that the official binaries don't do this, so homebrew shouldn't either. If that is homebrew's decided policy then it would make sense to close this PR. However, this provides a good explanation for why the official python binaries don't do this: they want one binary package for all macOS back to a fairly old version. But if I understand how homebrew works, then that isn't as much an issue for this change: bottles are OS version specific, and otherwise you build on local. So it sounds as if that issue could be sidestepped. If there are concerns about enabling these flags on older platforms or older versions of Xcode, perhaps we could make their application conditional on Xcode and OS version? Then at least newer platforms like Big Sur with Xcode 12 could get the better performance. If that would make this change more likely to be accepted I'm happy to make that change. It would be helpful if someone could point me to some example formulae that also introduce such conditionals. Note that at least the Ubuntu 20.04 debian packaging of python-3.8 does use LTO and PGO, but does so manually, rather than with |
|
@Homebrew/core thoughts here? Since we support back to 10.14 with individual bottles vs a single installer on 10.9 for the python.org installer, would these flags make sense in Homebrew at this point? |
|
Yes, it does seem like upstream's reasons for not enabling these flags don't apply here. It also seems like we have some pretty compelling reasons (56 of them, in fact) to enable these flags. It would be a shame not to take advantage of the fact that our bottling infrastructure is more specialised than what python.org uses to produce their installer. There is this cryptic but seemingly related comment in the linked discussion I want to make sense of, however: #45337 (comment) |
That is an interesting comment. Personally, my motivation here is that I use homebrew python to drive SCons for the mongodb build on macOS. SCons has its charms but speed is not currently among them, so anything that may accelerate it is valuable to me. Experimentation showed that enabling these flags in our local linux builds of python had significant performance advantages for certain SCons builds, and it seemed worthwhile to take the result of those findings over to homebrew, if possible. "Production" means different things to different people: I at least consider homebrew python to be a production level component of my macOS development environment. |
|
If the only reason is compatibility I am fine with deviating (with some documentation) from the upstream default. |
There was a problem hiding this comment.
This, on the other hand, needs to be reported to the Python developers. If the build fails without it, it's a Python bug. Please add a comment above that line with the Python bug link, so we can check whether it's safe to remove in the future.
There was a problem hiding this comment.
Sure, I'll get that reported to them and will add the bug link here. Meanwhile if we are going to continue with this workaround until the bug is fixed, is the /usr/bin portion here OK? Or is there an expansion I should be using to reach into the CLI tools? Using xcrun? I don't know enough about homebrew to know.
There was a problem hiding this comment.
/usr/bin is fine (but is a full path necessary, though?)
There was a problem hiding this comment.
LLVM_AR=ar seems to work fine for me locally../configure flags when I wrote this.
There was a problem hiding this comment.
My recollection is that I did need it and your experiment seems to confirm. Do you want me to double check? Or OK to move forward with the explicit path without further work?
There was a problem hiding this comment.
Filed https://bugs.python.org/issue43109. I'll get the link added to the PR, along with the comment changes requested elsewhere.
What form should this documentation take, and where should I add it? |
A comment above the configure flags along the lines of "deviating from default because we don't need backwards compatibility" is good enough iyam. |
20ce264 to
f60c3df
Compare
|
Updated with requested comment changes for |
f60c3df to
840ab28
Compare
|
I just merged #68528, so I rebased this to incorporate those changes. Also, included a revision bump and edited a comment a bit for readability. |
This is done using the configure flags
--enable-optimizations and --with-lto
840ab28 to
e4bd2e9
Compare
|
@carlocab - Anything else you need from me on this? |
|
@acmorrow Just your patience as we wait for CI to finish 馃槃 |
|
ARM: Big Sur: Catalina: Mojave: Most of these are documented in #66450. The ones that don't look spurious (e.g. |
Hah, sure thing. I'm very familiar with that kind of waiting. Looks from your recent comment though that the results are as expected. |
|
@carlocab, @fxcoudert, and @dtrodrigues - Thanks for all of your help getting this merged. Homebrew is a great project and a critical part of my development environment. I rely on it daily and it was fun to have an opportunity to participate in its development. I hope I've made python just a little faster for all of your users. PS @carlocab - I've noticed that new bottles don't seem to have gone out that incorporate this change. Not sure if that is just normal latency in the process or something else. |
|
Hmm, that's weird. The new bottles should've incorporated that change. How do you verify whether Python was built with the optimisation flags? |
|
Ah, I think I failed to clean up properly after putting together the PR and was pinned to older state. Sorry for the false alarm. To answer your question about how to see how it was built: |
6 participants
brew install --build-from-source <formula>, where<formula>is the name of the formula you're submitting?brew test <formula>, where<formula>is the name of the formula you're submitting?brew audit --strict <formula>(after doingbrew install <formula>)?Recent python offers flags to enable PGO and LTO which hopefully results in a faster Python interpreter. This PR adds the required flags to the homebrew
configureinvocation. It also papers over a macOS specific issue where theconfigurescript believes it needsllvm-arwhen building--with-lto. I don't think that is actually true on macOS, as Xcode doesn't providellvm-ar. I wasn't sure if there was an expansion like#{MacOS.sdk_path}that would be a better prefix than/usrwhen applying this override.