-
-
Notifications
You must be signed in to change notification settings - Fork 652
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
remove support for using the pants native toolchain with distutils Extensions in setup.py #7126
remove support for using the pants native toolchain with distutils Extensions in setup.py #7126
Conversation
This works locally (arch linux) because again distutils loves to try to find your compilers for you (much more than it loves letting you inject your own). I would wait to review until CI is green, although this is a small PR. We should honestly probably just remove all testing for |
Just removed all testing for native |
I'm reverting this to WIP status as I think we can clear up quite a lot of testing in one fell swoop while reducing CI time by a nontrivial amount. |
723e0fd
to
e35a200
Compare
Moved almost all of the |
14313e0
to
b2175b7
Compare
This should be reviewable now. The reason I chose to do all of:
Is because in removing this support, we shouldn't be continuing to test native code embedded in local dists, which allows us to cleanly separate the ctypes testing (the platform-specific behavior) from the rest. Many of the I am extremely receptive to comments or notes on how this can be split to make it easier to review, but I also am willing to say I will immediately fix any issues that arise with the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very happy we're removing this feature.
I wasn't really sure how to effectively review this. It's a large PR and I'm not very familiar with the code, beyond the C/C++ Py3 support I had added that I am happy to sign off on removing by getting rid of the module. Everything looked reasonable to me, but this is not my best area and you may want another review before merging.
class HelloTest(unittest.TestCase): | ||
|
||
def test_hello_import(self): | ||
self.assertEqual('hello!', hello.hello_string()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yay pure functions!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also a sly reference only to myself to one of my favorite songs https://www.youtube.com/watch?v=QYbn8NvPEtY
As this PR is blocking several others mentioned in the above and seems stable, I'm inclined to want to merge it sooner rather than later. I'm going to merge this now -- if it breaks anything it should be reverted immediately (ideally it should help reduce some CI time). |
Problem
Resolves #7016 (see mirrored
pants-devel
post). This unblocks #6855 and #7046, as well as further work on #7122.Closes #5661, closes #6841. I also made a long, long-overdue github project for native code.
Solution
Result
Further native backend iteration is unblocked.