-
-
Notifications
You must be signed in to change notification settings - Fork 8.7k
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
OpenMP detection is still broken on MacOS #8946
Comments
Example of configure that actually works: 2005m/kit#36 (comment) |
Thanks for tagging me and for the additional details. Are you interested in using your knowledge of this area to improve the project by opening a pull request? If not @trivialfis I'd be happy to look into this in a few days. |
@jameslamb Thank you for responding! I am interested to have this fixed so that we can have |
Thank you for raising an issue! As suggested by @jameslamb , would be great if you can submit a fix, your expertise in this area is really appreciated. |
@barracuda156 Thanks for raising the issue. As you noticed, currently XGBoost has only been tested with the Apple Clang + Homebrew combination. It will be great if you can make it work with Macports. |
UPD. Tests mostly pass, there is one failure on
I will look into this a bit more. In most cases these are fixable. (It was quite bad initially due to P. S. This is without OpenMP, but it is inconsequential for the issue. |
Thank you for looking into this. Out of curiosity, is mac ppc still available? |
@trivialfis Apple obviously dropped PPC support (starting from 10.7 ppc slices are gone, ppc64 dropped with 10.6), but macOS PPC is still used – as long as hardware is available, at least. |
I'm not sure if we can fix it though, or if the fix is worth the effort. None of us have access to such device, it's very likely that the next commit will just break it, especially now that xgboost is using c++-17 (CRAN is using c++-17 by default as well, AFAIK). |
@trivialfis Oh, don’t worry about C++17 is fully supported on macOS 10.4+ and on ppc/ppc64. No problems here. P. S. Could you take a quick look at this? #8947 |
Regarding the static assert, we can change |
@trivialfis Quick update: I fixed configure script for Macports environment, it works with GCC now, but I may need a bit of your assistance to write it neatly for a general case. Macports installs P. S. As you can see, linking works now:
|
unfortunately, that's beyond my knowledge. I don't have a mac device and am not familiar with its environment. Maybe a quick hack using |
I know what should be done for compiler so that OpenMP works, what I am less sure of is how to rewrite that chunk of code in configure optimally. Let me try something after I sleep. If that works locally I need someone to confirm it also works with Homebrew as well. |
Thank you for working on this! @hcho3 would be great if you can have a look when you are available. ;-) |
Just for the reference, here is a temporary fix for Macports: https://github.com/macports/macports-ports/tree/master/R/R-xgboost/files This, obviously, cannot be used here, but that logic works – it just has to be adapted conditionally. |
@barracuda156 I notice that the Portfile is patching in the prefix paths. How should we handle prefix paths in the vanilla |
@hcho3 I think we can use the default prefix for Macports case (
So the logic will be: 1) test for Homebrew, if true, use existing configure, if false, fallback to using Macports; 2) test for compiler being used, if Clang, use Clang-specific flags, if GCC (or just |
Homebrew GCC should work with My plan now is to test the following scenarios in order:
@barracuda156 For GCC, do we need to specify |
That will make sense. (BTW yeah, I have set Macports’s
Let me try throwing that out and see if it still works. Theoretically, GCC only needs |
@hcho3 @jameslamb I recently learned that there's an openmp macro in autotools |
@trivialfis I guess it merely checks if OpenMP is supported by the compiler, but does not set correct flags. So to make it work, flags should still be conditional, otherwise either GCC builds will break or Clang ones. |
It does supply the
|
This is needed for both compilers, but Clang will likely need |
Ah, thank you for sharing. |
@trivialfis I assume if |
Despite an earlier PR claiming to fix it: #8684
It is still broken, at least with GCC:
Moreover, it gives a wrong recommendation (
libomp
is Clang-specific).P. S. IMO it is a bad wording to use “should“ for an optional package manager. One does not need any –
libomp
, while not needed at all with GCC, can be installed directly. Also, there is Macports which has it, not just Homebrew.The text was updated successfully, but these errors were encountered: