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

remove support for using the pants native toolchain with distutils Extensions in setup.py #7126

Conversation

cosmicexplorer
Copy link
Contributor

@cosmicexplorer cosmicexplorer commented Jan 22, 2019

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.

@cosmicexplorer
Copy link
Contributor Author

cosmicexplorer commented Jan 22, 2019

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 python_dist()s with native Extensions in this PR as well, as it contributes significantly to our CI time (this is again because distutils mysteriously makes everything incredibly slow), and it appears unused. Please let me know if that sounds appropriate.

@cosmicexplorer
Copy link
Contributor Author

Just removed all testing for native Extensions in setup.py because that seemed like the right thing to do.

@cosmicexplorer cosmicexplorer changed the title remove support for using the pants native toolchain with distutils Extensions in setup.py [WIP] remove support for using the pants native toolchain with distutils Extensions in setup.py Jan 22, 2019
@cosmicexplorer
Copy link
Contributor Author

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.

@cosmicexplorer
Copy link
Contributor Author

cosmicexplorer commented Jan 23, 2019

Moved almost all of the python_dist() integration tests into unit tests and made a followup ticket for doing the same for the ctypes testing in #7128. Also made a native code github project! Will un-WIP this when it goes green.

@cosmicexplorer cosmicexplorer force-pushed the remove-setup.py-native-toolchain-support branch from 14313e0 to b2175b7 Compare January 24, 2019 18:17
@cosmicexplorer cosmicexplorer changed the title [WIP] remove support for using the pants native toolchain with distutils Extensions in setup.py remove support for using the pants native toolchain with distutils Extensions in setup.py Jan 25, 2019
@cosmicexplorer
Copy link
Contributor Author

This should be reviewable now. The reason I chose to do all of:

  1. remove the pants native toolchain from local dist building (in python_native_code.py and build_local_python_distributions.py)
  2. move the ctypes testing into a native/ subdirectory (see convert ~all of the ctypes integration tests to unit tests in the native backend #7128 for followup, scheduled to happen right after fetch native libraries from python requirements, e.g. tensorflow to define custom operators #7046 is in)
  3. convert most of the python_dist() integration tests into unit tests

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 python_dist() integration tests specifically used native code, which we are no longer supporting, so we would have had to drastically change those tests anyway. In the process of replacing them I realized that we could convert them all into unit tests, so the choice was of either rewriting the tests as integration tests again or as unit tests, and picked the latter.

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 python_dist() support as a result of this PR, and allow reviewers to avoid having to look into this too deeply. Note that the changes in src/ are minimal and that should make this significantly easier to review.

Copy link
Sponsor Member

@stuhood stuhood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

Copy link
Contributor

@Eric-Arellano Eric-Arellano left a 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())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yay pure functions!

Copy link
Contributor Author

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

@cosmicexplorer
Copy link
Contributor Author

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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
3 participants