GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | |||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | |||||||
GeezerVison is showing github.com — bpo-32593: distutils UnixCCompiler: remove FreeBSD special case by vstinner · Pull Request #5233 · python/cpython Conversation
|
|
@bapt: This PR is a follow-up of your #5232 (review) comment. Would you mind to review it and test it? |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
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) |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
"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. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
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. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
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. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
@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 :-) |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
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. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.