[Lldb-commits] [PATCH] D85539: [lldb] Extend builder to pass the TRIPLE spec to Make

Thu Aug 13 05:12:21 PDT 2020

labath added a comment.

The part which bugs me about this is that the first thing that Makefile.rules will do with the triple we've painstakingly constructed here is it will decompose it into individual components, and then do some more matching on them, including running some xcrun commands, which are already being run here. I do think that doing this in python is a good idea (I believe the main reason a lot of this stuff is done in make is so that running "make" would just work, but we are way past that point now), but if we're going to do that in python, how about we move all of that to python? It seems the only effect of the TRIPLE argument is to add `-target $(TRIPLE) -m$(OS)-version-min=$(VERSION)` to CFLAGS. That should be easy to achieve using the information that `construct_triple` has available.

In D85539#2203251 <https://reviews.llvm.org/D85539#2203251>, @JDevlieghere wrote:

> In D85539#2203247 <https://reviews.llvm.org/D85539#2203247>, @friss wrote:
>
>> `lldbremote.py` looks awfully Apple-specific, yet it is going to be called from the generic code. Is that not an issue?
>
> It's not an issue because it checks for the Apple SDK, but I could hoist that out of the helper to make it more obvious.

How about making an abstract/dummy `getTripleSpec` (or whatever) method in the base builder class. Then the darwin builder could override it do to its thing, and all of this code could reside in builder_darwin.py. If any other platform decides it needs to set the triple argument, then it's likely it will do that via some completely different method.

================
Comment at: lldb/packages/Python/lldbsuite/test/lldbremote.py:37-39
+        triple_platform = apple_sdk[:apple_sdk.find('.')]
+        if triple_platform == 'iphoneos':
+            triple_platform = 'ios'
----------------
when dealing with triples, I believe this field is usually called the "os", not "platform".

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85539/new/

https://reviews.llvm.org/D85539