[Lldb-commits] [PATCH] D26757: Fix broken escaping of commands in the build

[bryant via lldb-commits]

bryant added inline comments.


================
Comment at: CMakeLists.txt:46
+        COMMAND
+           ${PYTHON_EXECUTABLE} ${CMAKE_CURRENT_SOURCE_DIR}/scripts/finishSwigWrapperClasses.py
+               --srcRoot=${LLDB_SOURCE_DIR}
----------------
You can reduce diff noise by leaving formatting changes for a separate patch.


================
Comment at: scripts/CMakeLists.txt:38
+  COMMAND
+    ${PYTHON_EXECUTABLE} ${CMAKE_CURRENT_SOURCE_DIR}/prepare_bindings.py
+      ${framework_arg}
----------------
Move this back up.


================
Comment at: scripts/Python/prepare_binding_Python.py:222
+    )
+    logging.info("running swig with: %r", command)
 
----------------
You can reduce diff noise by limiting your changes to removing the %s. So,

```python
    # Build the SWIG args list
        options.swig_executable,
        "-c++",
        "-shadow",
        "-python",
        "-threads",
        "-I" + os.path.normcase(
            os.path.join(options.src_root, "include")),
        "-I" + os.path.normcase("./."),
        "-D__STDC_LIMIT_MACROS",
        "-D__STDC_CONSTANT_MACROS"]
    if options.target_platform == "Darwin":
        command.append("-D__APPLE__")
    if options.generate_dependency_file:
        command.extend(["-MMD", " -MF", temp_dep_file_path])
    command.extend([
        "-outdir", config_build_dir,
        "-o", settings.output_file,
        settings.input_file
    ])
    logging.info("running swig with: %s", command)
```


================
Comment at: scripts/Python/prepare_binding_Python.py:229
         stderr=subprocess.PIPE,
-        shell=True)
+    )
     # Wait for SWIG process to terminate
----------------
ldrumm wrote:
> granata.enrico wrote:
> > This worries me a little bit.. Are we sure we are not in any way relying on the shell in executing the command line?
> The features of the shell are not used in this expression at all, and the environment is identical to the previous invocation.
`shell=False` for both python 2 and 3: https://docs.python.org/2/library/subprocess.html#subprocess.Popen ; https://docs.python.org/3/library/subprocess.html#subprocess.Popen , unless I've missed your meaning.


https://reviews.llvm.org/D26757