This is an archive of the discontinued LLVM Phabricator instance.

cmake: add -DLLDB_ALLOW_STATIC_BINDINGS=1, defaults off
AbandonedPublic

Authored by tfiala on Nov 18 2015, 2:58 PM.

Details

Summary

This change does the following:

  • Adds a cached LLDB_ALLOW_STATIC_BINDINGS variable, defaults to off/false
  • If specified, flips on the --allow-static-binding flag to prepare_bindings.py
  • Removes the REQUIRED marker in cmake-based swig discovery, but adds a fatal error for cmake if swig isn't found when LLDB_ALLOW_STATIC_BINDINGS is also not set. i.e. unless -DLLDB_ALLOW_STATIC_BINDINGS=1 is specified on the command line, not finding swig will fail the cmake step as before.

As noted in a prior thread, I will be happy to rip this out once we no longer require swig via other means. This can be considered temporary until that time.

Diff Detail

Event Timeline

tfiala updated this revision to Diff 40564.Nov 18 2015, 2:58 PM
tfiala retitled this revision from to cmake: add -DLLDB_ALLOW_STATIC_BINDINGS=1, defaults off.
tfiala updated this object.
tfiala added reviewers: labath, zturner, spyffe.
tfiala added a subscriber: lldb-commits.

BTW I've tested this with the static bindings we use on OS X on Ubuntu 14.04 with a clang 3.6 build.

zturner edited edge metadata.Nov 18 2015, 3:04 PM

If nobody on the Apple side is using CMake, then I'm still not clear why we need this. Everyone using CMake already will continue to work fine using dynamically generated bindings, right?

I still don't agree with the need for the static bindings in any configuration, but if it's entirely on the Xcode side then there's not much I can do about it. But I really don't want this in the CMake build, and nobody else who depends on the CMake build has spoken up saying they need this.

If nobody on the Apple side is using CMake, then I'm still not clear why we need this. Everyone using CMake already will continue to work fine using dynamically generated bindings, right?

I'm at Apple. I build Linux. I use cmake.

I still don't agree with the need for the static bindings in any configuration, but if it's entirely on the Xcode side then there's not much I can do about it. But I really don't want this in the CMake build, and nobody else who depends on the CMake build has spoken up saying they need this.

So you can't install swig on your Linux machines either?

I really can't wrap my head around why we need this. What is the problem with installing swig, given that we need to install other software already?

The build instructions here [http://lldb.llvm.org/build.html#BuildingLldbOnLinux] say you need to install swig, libedit, python, and ninja. You need either git or svn too. Why must we insist on supporting this configuration when it is not necessary?

I want the CMake build to build one way.

(And I also work on cmake-based OS X lldb when there are issues that break there).

Keep in mind this is an opt-in flag. No developers get the behavior unless they specifically ask for it.

I'm adding it because I specifically need it.

So you can't install swig on your Linux machines either?

I really can't wrap my head around why we need this. What is the problem with installing swig, given that we need to install other software already?

Zachary, I appreciate your dislike of the idea. And I have totally modified my approach to this to minimize its impact so that it is a no-op for everyone else. And I am happy to remove this when we have an alternative to not require swig.

The build instructions here [http://lldb.llvm.org/build.html#BuildingLldbOnLinux] say you need to install swig, libedit, python, and ninja. You need either git or svn too. Why must we insist on supporting this configuration when it is not necessary?

I want the CMake build to build one way.

You are being obstructionist if you go with that statement. You are not impacted by this change, so attempting to shoot it down based on you not liking it for reasons that your team doesn't care about is not a position that makes sense. If I was forcing you to do something different, then we have something to work through if it caused undue harm. When I need to get something done, and I'm tweaking it a lot to appease you, and you still come back with "no, I don't like this so, even though I'm not the one who has a requirement for it, I'm going to say I don't want it" seems counter to an supporting multiple groups with different objectives in the same source base.

It's not that I don't like it. I mean, it used to be, but it's a minor change. At this point it's about that I've repeatedly asked for some kind of reasonable explanation about why swig is a contentious dependency, and the most I've gotten is "things are about to change". I don't even know what that means, and plus it sounded like it was a statement about OSX. But now all of a sudden it's abotu Linux too. What do I make of that?

It's not that I don't like the change, it's that I don't want to add complexity to the build system for something that I simply do not understand the value of. Installing swig is one command line on Linux, and one or two command lines on OSX. So why do we need to add 15 lines of CMake code so that people don't have to do a one-time execution of one or two command lines the first time they ever check out and try to build LLDB?

That's what I don't get. Maybe there's a reason, if so I'll LGTM it in a heartbeat, because it is a simple change. But if so it hasn't been clearly communicated.

zturner accepted this revision.Nov 18 2015, 3:38 PM
zturner edited edge metadata.

Anyway, just check it in. The content of this patch is not worth arguing about. What is worth arguing about, however, is that changes need to have some kind of reasonable justification. "I don't want to install swig" is not -- to me -- a reasonable justification. "I *need* to be able to not install swig" is a reasonable justification. That's what I've been trying to get out of someone, but it hasn't come yet, despite many many requests.

This revision is now accepted and ready to land.Nov 18 2015, 3:38 PM

What is worth arguing about, however, is that changes need to have some kind of reasonable justification.

I strongly disagree with this statement from a procedural point of view. It is just an invalid expectation. We are a bunch of commercial companies that work in a competitive environment. This goes for Google, Apple, Qualcomm, Sony, etc. I do not need a justification from each of these companies that explains items happening behind closed doors that motivate the desire for functionality in LLVM, clang, LLDB or any other open source project.

For example, Microsoft has an excellent debugger. Truly. World class. Google is implementing a debugger to work on the Microsoft platform. I don't need to know why that is the case. It's totally fine that I don't know why. It is not a requirement for me to know why. Even if this requires changing support to add Python 3.5 (that doesn't break others), even if our directory structure gets 30 - 50+ characters deeper than it was before, whatever. This is all okay procedurally speaking. I don't have the prerogative to require knowing all those details.

I made it clear here what goal I (and by I, we're talking Apple now) am trying to achieve:
http://lists.llvm.org/pipermail/lldb-dev/2015-November/008802.html

In particular:
"The primary goal here is to remove the requirement of having swig on the
system (e.g. for builders and most developers)"

You keep asking the question and not liking the answer, but the goal is to "not require swig." The approach has been watered down so that this only happens when opted in, but that is the goal. And this is Apple's goal. You may not like the goal, and I am not going to try to persuade you to like it. But that is the goal.

At this point, it is entirely opt-in, and off by default. Nobody other than Apple has to maintain the static bindings. So it is our goal, we implemented a way to do it, and we are responsible for maintaining it. If a by-product of this breaks somebody who didn't opt into this, then I think that is something I'd need to look at us addressing.

But no, asking for repeated justification for *why* that is Apple's goal beyond what I've already said (i.e. making it simpler for a casual developer to get a machine that can build lldb) seems both an unreasonable expectation and completely unnecessary to discuss. Again, I'm happy to rip out the manner in which we're achieving that if something considered better comes along (which your bindings-as-a-service idea sounds like a potentially promising avenue to look at). But I don't see there being a requirement to argue a goal that doesn't hamper the existing developer community.

So, this is the last thing I'm going to say on this thread.

What I'm fighting for here is consistency. We seem to have the same goal (making LLDB easier to build for the casual developer) but a different understanding of how to go about accomplishing this. From your perspective, you want to reduce the number of dependencies required to build LLDB. From my perspective, I want to reduce the number of *different* ways of accomplishing the exact same goal. And reducing a dependency is not a fair trade for increasing the number of ways of doing the same thing.

Yes I'm drawing a line in the sand here, but it's because this has to stop. We -- as a community -- have to stop adding many ways to do the exact same thing unless it is necessary. I don't care if it makes things slightly more difficult for someone, the simplicity comes from the consistency. You don't have to look hard to find examples of we do things differently on our own whims, disregarding established conventions of how to do the same thing.

We have a different coding standard than the rest of LLVM.

We have a different build system than the rest of LLVM.

We have a different directory structure than the rest of LLVM.

We have a different test infrastructure than the rest of LLVM.

*These* are the things that keep casual developers away, not installing swig on their machine. I know this for a fact, because every time an LLVM developer breaks the buildbot, they send me nasty messages on LLVM's IRC about why it's so hard to run the test suite, how to interpreter the results, why doesn't it just use lit, why are my tab settings all messed up when I open the file in an editor, what is this weird directory structure, I can't even build!

Your goal is to remove the swig dependency, but my goal is to converge on a set of practices and conventions that are the same across all platforms. That's the ease that I'm looking for. Maybe this will be possible once the swig service is up and running and everyone can use the latest version of swig. From my perspective, it's possible right now if we use on-the-fly bindings. That's why I'm arguing this. We, as a community, have to share each others' pain in order to reap each others' benefits. That's how this whole process works.

In any case, lgtm. Check in this change. But please consider this in the future. One way, unless there's no other way.

Not long ago we rejected dynamic dependency on curses(3), as a fall-back for NetBSD (without libpanel(3) in the 7.0 release). Why doing the similar thing with swig is fine?

Why are we abstracting swig when there is no other option viable?

Is possible to improve this approach of finding swig? We are looking for it in CMake and later in Python?
find_package(SWIG), --find-swig.

Maybe just add a single switch in CMake to enable or disable it without options.

Please explain what does static bindings mean here.

Not long ago we rejected dynamic dependency on curses(3), as a fall-back for NetBSD (without libpanel(3) in the 7.0 release). Why doing the similar thing with swig is fine?

Why are we abstracting swig when there is no other option viable?

Is possible to improve this approach of finding swig? We are looking for it in CMake and later in Python?
find_package(SWIG), --find-swig.

Maybe just add a single switch in CMake to enable or disable it without options.

Please explain what does static bindings mean here.

static binding = one person runs swig to generate LLDBWrapPython.cpp and lldb.py, then they check these files into the upstream. Nobody else has to run swig.

Dynamic binding aka on-the-fly-binding = only the swig interface file is checked in. There is a build step that automatically runs swig locally on every developer's machine when the interface file changes and automatically generates LLDBWrapPython.cpp and lldb.py at build time.

I dislike delegating jobs (or hiding problems) of CMake (assuming that autoconf/gmake is already dead) to external scripts like Python ones.

In the goal, I plan to add LLDB in the NetBSD base (next to LLVM and Clang) where there is no Python at all. In FreeBSD there is no Python in base as well. To state it stronger, there might never be Python in the base.

I would expect to just have a single switch like -DLLDB_ENABLE_PYTHON, while I don't understand -DLLDB_ALLOW_STATIC_BINDINGS.

When I will catch up with platform support I'm considering to prepare bindings in Lua, as this is the standard scripting language in NetBSD (we script with it kernel, packet filter, userland etc). Lua offers native C/C++ bindings (as there is no need for intermediate tool) by design and is a dynamic language close to Python. But this isn't plan for this year.

For now please leave an option that end user to disable Python. Otherwise erasing it will be done downstream without benefit for the project.

For now please leave an option that end user to disable Python. Otherwise erasing it will be done downstream without benefit for the project.

Nobody is talking about removing the option to disable python.

The static binding binding option is ignored (i.e. irrelevant) if python is disabled.

Sure, I'm just giving general feedback.

From a user perspective I want to get yes/no information whether I need swig or not, if so what version. If we allow Python, I don't see any reason to not allowing swig. if some version gives you problems (license, bad code generator), please enforce a certain one.

I would reject this patch.

Kamil, the situation is a little complicated, please read the following threads for some context:

http://lists.llvm.org/pipermail/lldb-dev/2015-November/008802.html
http://lists.llvm.org/pipermail/lldb-dev/2015-November/008829.html
http://lists.llvm.org/pipermail/lldb-dev/2015-November/008851.html

TL;DR

  1. -DLLDB_DISABLE_PYTHON=1 is not going anywhere, you can still use it.
  2. Yes there are license problems with certain versions of swig, but only for some people, and other people *must* use a different version, so there is not currently a single enforceable version that works for everyone.

If you have further comments of a general nature, it's probably best to continue discussing them on one of the aforementioned threads (or create a new thread on lldb-dev)

tfiala abandoned this revision.Nov 18 2015, 10:39 PM
labath edited edge metadata.Nov 19 2015, 2:25 AM

A bit late to the party, but anyway... I would change the detection logic here:
Instead of ALLOW_STATIC_BINDINGS, have USE_STATIC_BINDINGS. If it's set, don't even bother checking for swig's presence and use the static bindings. If it's not set, then detect swig, and make it an error if it's not found.
I like this more because then the build will not do something completely different depending on whether it finds some binary on your system or not. I don't feel so strongly about that as in the case of curses (this is the dynamic dependency kamil was referring to) because this only kicks in if some option is specified, but I do think this provides more visibility into what the build system is doing.

tfiala added a subscriber: tfiala.Nov 19 2015, 7:34 AM

Moot point now, but thanks.

The reason for the flag name is that the prepare_bindings.py script as it
stands will always generate the bindings with swig if a swig is either
specified or allowed to be found in some common places. So the
"USE_STATIC_BINDINGS" would be misleading --- they wouldn't necessarily be
used - only when a swig could not be found. So that flag literally
controlled whether static bindings were even permitted (i.e. allowed) to be
used, not that they would be.