[lldb-dev] [Lldb-commits] [lldb] r253317 - Add Pythonic language binding wrapper generation script.

[Todd Fiala via lldb-dev]

On Wed, Nov 18, 2015 at 10:34 AM, Zachary Turner <zturner at google.com> wrote:

>
>
> On Wed, Nov 18, 2015 at 10:00 AM Todd Fiala <todd.fiala at gmail.com> wrote:
>
>> Checking in the static bindings is no different than projects checking in
>> autoconf config baked scripts so that the vast majority of people don't
>> need to run autoconf just to get a setup that rarely changes.  There is
>> precedent for this going back a long way on open source projects.
>>
>> I'm backing off having anyone else use them if they don't want, and we
>> (Apple) will keep those up to date.  Nobody else will use them.  Totally
>> fine.
>>
>> On the swig-as-a-service front, I think the idea is interesting but there
>> are several practical holes with that:
>>
>> * What does one do when the server is unavailable?  Needs to be a
>> local-only backup.  So whatever service that provides isn't a guaranteed
>> working solution (think on an airplane, network outage, other server
>> outage, etc.)
>>
>
>> * Security: building code that has other code injected in it by another
>> server. Safe? Attack vector?  I could argue so is a git/svn repo
>> susceptible to this, so maybe this isn't a huge deal, but it's big enough
>> and smells enough like "introduce random unvetted code that can't be
>> reviewed as easily as a VCS tag" that I doubt we would ever use this in
>> practice.
>>
>> * Security 2: what is the service really running?  Not obvious on the
>> build machine accessing the service.
>>
> The end result of the service is a copy of LLDBWrapPython.cpp and lldb.py
> that you check into the repo.
>

Ah, I see.  That definitely sounds like an improvement from my
interpretation :-)


> You still have a chance to diff this source code against the repository's
> copy before committing, same as you would if you had swig locally.
>

Yeah, non-issue at that point.


> Vast majority of changes to SB interfaces are going to be the addition of
> a couple methods, or maybe a class, and the diff should be very easy to
> look at and understand.
>

Yes.


>
>
>>
>> > In all of these cases (except the proposed), the matrix choices are
>> justifiable because they are there to support a hard requirement of
>> someone's environment, and I do not think we should grow for anything that
>> is not also a hard requirement of someone's environment.
>>
>> I'm going to call that overreaching.  We are not in the business of
>> dictating that one of the developers of the code "should not do something
>> unless there is a hard requirement."
>>
> I'm not saying you shouldn't do *anything* if there's not a hard
> requirement.  I agree that's overreaching.  I'm saying we should not
> increase the number of ways of doing *the same thing* unless there is a
> hard requirement.  Especially if one of the ways of doing the same thing
> exists solely to save someone from running one command to install the
> package (sorry if I'm not doing justice to how difficult it is on OSX, the
> last time I did something on OSX it seemed fairly easy to install macports,
> and I thought it's a wildly common thing for people to already have
> installed).  For example, wouldn't people need to already have macports in
> order to install CMake -- a necessary component of building LLVM?
>

In the case of cmake, no. The cmake website provides a simple-to-add binary
that doesn't depend on anything else.  (Okay, I'm long winded here so
Enrico just beat me to the punch).

For swig, the solution is homebrew/macports.  The challenge with systems
like homebrew and macports is that they will install other packages that
are dependencies (sometimes build-time, sometimes runtime), typically get
them added to various paths, and (during the process), unintentionally hide
system libraries.  It is not uncommon for us to see crash reports on OS X
where Xcode or lldb is picking up the wrong python or other component that
was built by one of these package managers.  They're often good about
saying "this might cause trouble" in some obvious cases, but less so in
less obvious cases (like, a swig installing python bindings, which installs
a new shiny python, which hides the system one, which breaks the lldb
bindings).  It really is messier there.


>
>
>>  Apple wants to eliminate the need for people to *require* swig.  The
>> goal there is reducing the build requirements for the average person
>> building lldb, not growing it.
>>
> I agree, which is why it is so important to keep the number of different
> ways of building to a minimum.
>

Glad to hear the intent here.


>   It's the same reason that the autoconf build is being removed wholesale
> from LLVM and people have decided that CMake is the one true way.
>

(Coincidentally - I am in the process of trying to eliminate the old
make-swig-bindings.sh, which is still used by the Makefile/autoconf build
of lldb, and I saw the nice banner from llvm saying autoconf support is
going out for 3.9 while testing the replacement).


>   Because even if it isn't perfect for everyone, it works for everyone.
>

Unless you can't get swig on your system in a way that doesn't break other
items.  And then it doesn't work (as things stand now).


> And there is inherent simplicity in having fewer ways to do things, as
> well as reducing maintenance cost.
>
>
>>
>> So for the more common, casual lldb build environment where the developer
>> is not touching SB API, help me understand how reducing the need for swig
>> (without introducing the need for hitting another server) is increasing the
>> requirement load?  (Especially if we --- our local dev group sitting by me
>> --- maintains those static bindings)?
>>
>
> Well, we would need to disable static bindings on the OSX buildbots for
> starters, otherwise when someone not using static bindings makes a change,
> the buildbots break, and we cannot leave buildbots in a broken state.  So I
> assume that will still be possible.  So now you don't have a buildbot
> testing the static binding configuration.
>

I was actually going to add a verifier stage to the public OS X buildbot.


>
>
> Secondly, LLDB already has a problem (IMHO) of having too many things that
> only work for a few people, instead of having things that work for
> everyone.  It's gotten better, and even your work right now to port the
> Xcode build over to these new python scripts is helping to make that
> better.  So regardless of the outcome here, the end result will still be an
> improvement over before when Xcode build was using the shells cripts and
> CMake build was using python scripts.
>
> But I still think it's important to take a hardline against introducing
> new build configurations.  Static vs on-the-fly bindings are going to need
> different logic for getting the resulting code into the place where it can
> be compiled / imported, different logic for creating the symlinks, etc.
>

Yep - it was in the vicinity of 25 lines of python.  An extra option to say
"okay, only try this if there is no swig found", a few lines around "if we
can't find a swig, and we say it's okay to copy the static bindings, then
do it, otherwise fail", and a method to copy over the LLDBWrapPython.cpp
and lldb.py files.


> And now we're duplicating effort when one person changes the logic on
> their side but not the other side.
>
>
I'll offer again that we'll take care of the static bindings.  It's on us
if they're broken.


> I guess if this entire thing is hidden away in the Xcode build and
> everyone using the Xcode build wants things to work this way then I can't
> really argue with that, but I don't think we should provide the option to
> use static bindings in the CMake build.  (If anyone who uses the CMake
> build disagrees though, please speak up)
>

My intent here is to only provide the --allow-static-binding option on
cmake if it is explicitly turned on.  And it will be off by default.


Tell me more about the service idea.  How would you envision it working?

-- 
-Todd
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-dev/attachments/20151118/7f91ac0b/attachment-0001.html>