Page MenuHomePhabricator

Avoid creating lldb-python-scripts target more than once
ClosedPublic

Authored by aadsm on Aug 22 2020, 10:27 AM.

Details

Summary

This addresses the issue raised here https://reviews.llvm.org/rG02bf5632a94da6c3570df002804f8d3f79c11bfc
The finish_swig_python function might be called more than once so we need to make sure that the
lldb-python-scripts target is only created once.

Diff Detail

Event Timeline

aadsm created this revision.Aug 22 2020, 10:27 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 22 2020, 10:27 AM
aadsm requested review of this revision.Aug 22 2020, 10:27 AM
JDevlieghere requested changes to this revision.Aug 22 2020, 10:38 AM

The target should be created for everyone calling the finish_swig_python function. Let's say (not so) hypothetically I want to create bindings for Python 2 and Python 3. I'll call finish_swig_python, twice, once for each Python version:

finish_swig_python("lldb-python" "${lldb_python_bindings_dir}" "${lldb_python_target_dir}")
finish_swig_python("lldb-python2" "${lldb_python_bindings_dir}" "${lldb_python_target_dir}")

I want two corresponding targets, lldb-python-scripts and lldb-python2-scripts and two install targets. So what I proposed in D86235 is using ${swig_target}, which is the first argument to finish_swig_python to be part of the target name:

set(swig_scripts_target "${swig_target}-scripts")
set(swig_scripts_install_target "install-${swig_scripts_target}")

This will create lldb-python-script and install lldb-python-script for the first call, and lldb-python2-script and install lldb-python2-script for the second call to finish_swig_python.

This revision now requires changes to proceed.Aug 22 2020, 10:38 AM

Sounds good, will update. In my mind it would be easier to just install all configured python scripts by specifying a single distribution component.

aadsm updated this revision to Diff 287211.Aug 22 2020, 11:58 AM

Update to create 2 separate install targets

This revision is now accepted and ready to land.Aug 22 2020, 6:40 PM