[Lldb-commits] [PATCH] D26757: Fix broken escaping of commands in the build
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
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D26757: Fix broken escaping of commands in the build
bryant added inline comments.
Comment at: scripts/Python/prepare_binding_Python.py:222
+)
+logging.info("running swig with: %r", command)
ldrumm wrote:
> bryant wrote:
> > 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)
> > ```
> But `logging.info` is not a pretty printer - if the command fails for some
> reason we need to see why. `repr` allows this, and the diff noise is again
> minimal because that line is changing anyway and command is now a list, not a
> string
Yes, that was a typo. Keep %r too.
https://reviews.llvm.org/D26757
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D26757: Fix broken escaping of commands in the build
bryant added inline comments.
Comment at: CMakeLists.txt:53
+ --lldbLibDir=lib${LLVM_LIBDIR_SUFFIX}
+ ${FINISH_EXTRA_ARGS}
+VERBATIM
The indentation here could match the above.
https://reviews.llvm.org/D26757
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
