Perfect your code
With built-in code review tools, GitHub makes it easy to raise the quality bar before you ship. Join the 36 million developers who've merged over 200 million pull requests.
Sign up for free See pricing for teams and enterprisesbpo-32593: distutils UnixCCompiler: remove FreeBSD special case #5233
Conversation
the-knights-who-say-ni
added
the
CLA signed
label
Jan 18, 2018
bedevere-bot
added
the
awaiting merge
label
Jan 18, 2018
vstinner
added
the
skip news
label
Jan 18, 2018
vstinner
referenced this pull request
Jan 18, 2018
Merged
bpo-32593: Drop FreeBSD 9 and older support #5232
This comment has been minimized.
This comment has been minimized.
|
@bapt: This PR is a follow-up of your #5232 (review) comment. Would you mind to review it and test it? |
This comment has been minimized.
This comment has been minimized.
bapt
commented
Jan 18, 2018
|
I don't know how I can test this given after building with the patch I can't find any elf file with rpath assigned. 3 tests failed: 20 tests skipped: This is run on FreeBSD current FreeBSD 12.0-CURRENT #1 r326932M: Mon Dec 18 17:09:51 CET 2017 as a user (not root) |
This comment has been minimized.
This comment has been minimized.
|
"3 tests failed: test_gdb test_posix test_uuid" What are these failures? All tests are supposed to pass on FreeBSD. But I'm aware of an issue with test_uuid, a recent regression. |
vstinner
requested a review
from dstufft
Jan 18, 2018
This comment has been minimized.
This comment has been minimized.
koobs
commented
Jan 19, 2018
|
Originating issue for this code is issue 20767. Based on my memory and understanding, the code being proposed for removal was not added (in 3.6) for and was not specific to FreeBSD 9. Unless something has changed (eg: upstream @ clang/llvm and merged back to all clang versions used in all supported FreeBSD versions), it doesn't appear removing this code is relevant to removing 'EoL OS support' Having said that, if a (version) unconditional 'freebsd' special case is no longer required at all, removal is fine. |
This comment has been minimized.
This comment has been minimized.
bapt
commented
Jan 19, 2018
|
Interesting and thank you for this input, the thing is yes clang rejects -R but does not reject -Wl,-R which will pass the argument directly to the linker like gcc does (I just tested with clang 3.3 and earlier version). Looking at the rest of the code path, what was failing for that issue is/was the detection of clang as a compiler gcc compatible which is the only situation where -R can be passed to the compiler directly. So in my opinion this PR is right as this code should be removed (it is wrong), then in addition a proper fix would be added here: https://github.com/python/cpython/blob/master/Lib/distutils/unixccompiler.py#L237 by treating clang as gcc, btw the gcc detection https://github.com/python/cpython/blob/master/Lib/distutils/unixccompiler.py#L209 is wrong as well, in many systems, the gcc compiler is just named cc meaning this test will fail. |
This comment has been minimized.
This comment has been minimized.
|
@koobs: "it doesn't appear removing this code is relevant to removing 'EoL OS support'" Sorry, I abused this bpo number. I created this change as a follow up of #5232 (review) @koobs, @bapt: If the change looks good to you, would you mind to use the GitHub review to "Approve" this PR? By the way, I would be more confident if you both approve the change :-) |
This comment has been minimized.
This comment has been minimized.
bapt
commented
Jan 19, 2018
|
So after a deeper look, the change is not ok without a change in the code that finds if the compiler is gcc to make it accept clang as well. see my previous comment. |


vstinner commentedJan 18, 2018
•
edited by bedevere-bot
There is no need to have a special case for FreeBSD.
FreeBSD can share the same code than Linux.
https://bugs.python.org/issue32593