Move management of calls of "cmake --build" to setup_helper/cmake.py and refactor as a CMake class

[xuhdev]

No description provided.

[peterjc123]

Sorry for the late review. I didn't get why do we need this refactor? Could you explain this a little bit?

[ezyang]

Yeah, it would be really helpful to know the motivation behind the refactor. I personally think that one of our big antipatterns is how long our cmake invocations are; we really should strive to have as few arguments to cmake as possible (and not make it easier to add more and more arguments.)

[xuhdev]

The purpose of this PR is a step toward better integration of the Python setup scripts and CMake. Ultimately, I would like to allow build options to be directly specified using options specified in CMakeLists.txt (without explicitly being processed in Python setup scripts), allow users to adjust build options directly via cmake-gui/ccmake/editing CMakeCache.txt (currently users must take care of both options in CMakeCache.txt and environmental variables passed to setup.py and ensure they are consistent upon rebuild), and remove a lot of system and library environment checks in Python setup scripts while CMake can already handle them pretty well.

This PR makes it easier to manage CMake related code in Python setup scripts and paves a path towards these objectives. A well structured code base is important, because the next step is probably to look at one build option at a time PyTorch currently explicitly supports in setup.py.

@ezyang I agree with you that there are many issues with current cmake invocation. The objective I described above subsumes many cmake invocation issues, because only what users have specified will be sent to CMake as arguments.

[xuhdev]

@ezyang I should also stress that to address the long CMake invocation argument list, as stated above, I have to look into each build option one at a time. I would focus on those once we have some basic structure in Python setup scripts better organized.

[peterjc123]

LGTM, waiting for @ezyang's response

[ezyang]

OK sure.

[ezyang]

Things that you don't have to do this PR, but to think about for the future as you keep working on this code (a lot of these are preexisting conditions):

Python nits:

1. More docs: for every function, write a docblock that explains what the expected type and semantics of each argument
2. Let's try to use keyword arguments for long argument lists, like the invocation of CMake.generate

General design thoughts:

1. For BC reasons, we do need to keep supporting passing environment variables to invocations of setup.py (also, it is the easiest way to make setup.py invocations configurable in any case), so it will be worth thinking what the terminal state of the code will be, given this constraint.
2. We should figure out some sort of CI-based testing strategy for direct cmake invocation after the fact. Maybe something as simple as running ninja after setup.py and expecting this to be no-op.

[facebook-github-bot]

@ezyang is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

[facebook-github-bot]

@ezyang merged this pull request in 646a7f9.

[xuhdev]

@ezyang Thanks! Will keep these in mind.