allow for cmake installation #160
Conversation
|
@weaverba137 this PR appears to have trigged a bunch of MaskedArrayWithLimits-related test failures completely unrelated to the changes in this PR. Please investigate. |
| @@ -516,12 +516,16 @@ def build_type(self): | |||
| self.log.debug("Forcing build type: make") | |||
| build_type.add('make') | |||
| else: | |||
| print('cwd: ',self.working_dir); | |||
There was a problem hiding this comment.
Please remove bare print or switch to useing self.log.debug.
| proc = Popen(command, universal_newlines=True, | ||
| if 'cmake' in self.build_type: | ||
| proc = Popen(command, universal_newlines=True, | ||
| stdout=PIPE, stderr=PIPE, shell=True) |
There was a problem hiding this comment.
This seems messy / fragile where "cmake" requires two-commands-in-one and thus a string command with shell=True, while "make" uses a single-command-passed-as-a-list with default shell=False. The normal admonishment against shell=True [link] is due to security concerns about shell injection vulnerabilities from unsanitized user input strings, but in this case we have completely control over the commands created without any user-provided options, and thus I think both "make" and "cmake" could use string (not list) commands with shell=True to avoid this if/else. Thoughts?
There was a problem hiding this comment.
To further @sbailey's comment, did you actually test the commands with shell=False and come to the conclusion that it was impossible to run with shell=False?
I don't think anyone would object if we ran two commands, both with shell=False, if the one and only reason for shell=True is a ;.
There was a problem hiding this comment.
Did a test and found it would require two running two commands and the shell=True was chosen. Will try the setup.py solution @tskisner mentioned below and if so these changes will be unnecessary.
There was a problem hiding this comment.
That sounds promising. I was about to point out that you have if 'cmake' in self.build_type:, but you have not modified the code in any way to detect 'cmake' as the build_type.
|
Just throwing out another option- it is possible to have a This even supports setting environment variables to pass options to the underlying cmake call. This way the desiInstall tools can just run setup.py and not worry about what it is doing under the hood. It is just a question of where you want to put the extra logic- in the package or in the external tools. |
I think having cmake called by |
|
Requiring |
|
The specex build now uses python setup.py instead of cmake, so this change is no longer necessary. Closing this pull request and deleting the branch. |
This modification changes desiInstall such that if there is a
CMakeLists.txtfile in the top-level directory of a package, then it will use the commandscmake .; make -j 8to install. This currently is for specex but should work generally and can be modified as necessary in the future.