python3: Add C++ compiler support for distutils (on non gcc systems) #27649
Conversation
knedlsepp
changed the title
knedlsepp
changed the title
|
I just tried this out and it fixes the build of |
|
What bugs.python.org issue is this patch linked from? |
|
It's from http://bugs.python.org/issue1222585 should I add that in the commit messages? |
|
@knedlsepp in a comment in the code please. Also, what about http://bugs.python.org/issue1222585#msg242706? |
|
@FRidh: Thank you for the thorough look at this. This is something that I overlooked and I need to incorporate before we merge! |
| @@ -64,6 +64,14 @@ in stdenv.mkDerivation { | |||
| substituteInPlace configure --replace '-Wl,-stack_size,1000000' ' ' | |||
| ''; | |||
|
|
|||
| patches = optionals (!(stdenv.cc.isGNU or false)) [ | |||
There was a problem hiding this comment.
Is there a reason not to apply this for gcc?
There was a problem hiding this comment.
Well. The problem does not occur when using gcc, so I thought it would be best to make the intention clear. I'm not sure if applying this unconditionally might break something else, so I am basically being cautious.
There was a problem hiding this comment.
How I understand it is that it currently only works because gcc doesn't complain when compiling c++ code using $CC, clang does which is arguably the correct thing to do 😄 .
In cases like this I prefer not to use conditionals, making everything more consistent and the darwin build less brittle. (if the patch fails to apply the linux build won't break)
There was a problem hiding this comment.
Ok. In that case I should probably boot up a little NixOS VM and try if that also works fine. BTW: The python27 currently also does this only on !isGNU, should we maybe change that?
There was a problem hiding this comment.
@FRidh Just saw your post on the 2.7 version of this issue: #19585 (comment) what's your take on !isGNU?
There was a problem hiding this comment.
@LnL7 has a valid point but I would prefer to keep the Python interpreter as vanilla as possible and therefore only apply patch when really necessary.
There was a problem hiding this comment.
I generally prefer to minimize amount of "branchy" logic between platforms, if we can pull it off. I'd make this unconditional, adding a comment saying what we say here, i.e., that gcc is okay with it and clang isn't, and we're doing this so we don't bake in assumptions about weird behavior in specific compilers.
knedlsepp
force-pushed
the
fix-python3x-distutils
branch
3 times, most recently
from
2796f21
to
5228146
Compare
|
Please do not merge ATM, just broke it. |
knedlsepp
force-pushed
the
fix-python3x-distutils
branch
from
5228146
to
642c5ab
Compare
|
@FRidh: I patched the patch based on the comment you mentioned and did upload it to their issue tracker. My reworked PR now links to the patch I uploaded. I also put a comment in the code mentioning the issue. Did a little test with compiling a C++ |
|
@knedlsepp thanks! Is there anything that should be done with the 2.7 version of the patch? |
|
@FRidh Hm, we might: #19585 (comment) |
knedlsepp
force-pushed
the
fix-python3x-distutils
branch
from
642c5ab
to
81b1d4a
Compare
knedlsepp
force-pushed
the
fix-python3x-distutils
branch
from
81b1d4a
to
2bf1d0a
Compare
|
@FRidh: Could we merge this? While there might still be some open issues considering dropping the CFLAGS on 2.7, I think we should nevertheless get 3.x up and running. |
|
This will apply the patch unconditionally unlike your other patch.
|
|
I understand your point of wanting to keep the interpreter as vanilla as possible. As the motion in #27649 (review) however was mostly: Applying the patch unconditionally will be less painful for darwin maintenance, I went with that approach instead. I think the patch failing to apply for a maintainer only testing on linux could be seen as a useful information. The 2.7-patch was actually incorporated by veprbl, I only copied the fix. I wouldn't mind re-adding the conditional again if you think there's more value the vanilla python interpreter on linux. |
|
Have you tested numpy+cython with this? |
|
Closing in favor of: #39576 |
Motivation for this change
Python 3.x on darwin did not correctly distinguish between C++ and C builds, so we got errors relating to missing C++ headers. The fix is analogous to the one for python2.7 done in #19585.
This fixes #18194.
Things done
(nix.useSandbox on NixOS,
or option
build-use-sandboxinnix.confon non-NixOS)
nix-shell -p nox --run "nox-review wip"./result/bin/)