Skip to content
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

bpo-32593: distutils UnixCCompiler: remove FreeBSD special case #5233

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
5 participants
@vstinner
Copy link
Member

commented Jan 18, 2018

There is no need to have a special case for FreeBSD.
FreeBSD can share the same code than Linux.

https://bugs.python.org/issue32593

distutils UnixCCompiler: remove FreeBSD special case
bpo-32593: There is no need to have a special case for FreeBSD.
FreeBSD can share the same code than Linux.
@vstinner

This comment has been minimized.

Copy link
Member Author

commented Jan 18, 2018

@bapt: This PR is a follow-up of your #5232 (review) comment.

Would you mind to review it and test it?

@bapt

This comment has been minimized.

Copy link

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.
That said I run the testsuite both prior and after the patch on FreeBSD current with exactly the same results:
383 tests OK.

3 tests failed:
test_gdb test_posix test_uuid

20 tests skipped:
test_curses test_devpoll test_epoll test_msilib test_ossaudiodev
test_smtpnet test_socketserver test_spwd test_startfile
test_timeout test_tix test_tk test_ttk_guionly test_urllib2net
test_urllibnet test_winconsoleio test_winreg test_winsound
test_xmlrpc_net test_zipfile64

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)

@vstinner

This comment has been minimized.

Copy link
Member Author

commented Jan 18, 2018

"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 vstinner requested a review from dstufft Jan 18, 2018

@koobs

This comment has been minimized.

Copy link

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.

@bapt

This comment has been minimized.

Copy link

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.

@vstinner

This comment has been minimized.

Copy link
Member Author

commented Jan 19, 2018

@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 :-)

@bapt

This comment has been minimized.

Copy link

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 vstinner closed this Feb 1, 2018

@vstinner vstinner deleted the vstinner:distutils_freebsd branch May 29, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.