[Lldb-commits] [PATCH] D41902: Remove Platform references from the Host module

[Davide Italiano via lldb-commits]

The test Jim committed broke the public bots, so I went ahead and reverted it.


Committing to https://llvm.org/svn/llvm-project/lldb/trunk ...
commit 87eb2de0885d646e71d5848c1fa699b784bf5d2b
Author: Davide Italiano <ditaliano at apple.com>
Date:   Thu Jan 11 14:37:49 2018 -0800

    [testsuite] Remove a broken test which tried to find App in bundles.

    That never really worked, and the change associated isn't yet
    committed, so, let's try to make the bots green for now.


Let's make sure we add a test before we commit your change :)

Thank you!

--
Davide

On Thu, Jan 11, 2018 at 9:51 AM, Pavel Labath <labath at google.com> wrote:
> On 11 January 2018 at 16:22, Greg Clayton <clayborg at gmail.com> wrote:
>>
>>> On Jan 11, 2018, at 8:10 AM, Pavel Labath <labath at google.com> wrote:
>>>
>>> On 11 January 2018 at 16:04, Greg Clayton <clayborg at gmail.com> wrote:
>>>>
>>>>> On Jan 11, 2018, at 5:12 AM, Pavel Labath <labath at google.com> wrote:
>>>>>
>>>>> Thanks for adding the test Jim. I have confirmed that it passes with
>>>>> this patch applied (because the bundle is resolved during target
>>>>> creating, not launching).
>>>>>
>>>>> However, this did make me aware of the fact that this patch removed
>>>>> the bundle resolution logic from the launcher itself, so I'm going to
>>>>> add back that part.
>>>>
>>>> The platform really does need a part in this. Each platform might have different files. APK files for Android, .ipa files for iTunes, .app bundles for all darwin operating systems and more. So the platform really is the right place for this kind of logic.
>>>
>>> Yes, but this is in the host layer, which already assumes the
>>> knowledge of the host platform -- there no sense in trying to "launch"
>>> an APK file on iOS. It actually fits quite nicely, since the code that
>>> actually resolved the executable in the bundle was already present in
>>> the host layer.
>>
>> I am all for detangling the Host and Platform code. As long as we maintain the ability to resolve a binary inside a bundle when setting the executable in a Target, I am not picky where this happens.
>
> I fully agree with that. To elaborate on my previous statement:
>
> The code that handles the resolving of executables for debugging is in
> the Platform class and I'm not changing that.
>
> The code I'm editing is only used when you actually go on the exec()
> the executable. This only makes sense for executables referring to the
> host platform. For executables we launching for debugging, it doesn't
> even matter if we resolve stuff here, because it will already be
> pre-resolved. So, this only applies to other things we are launching,
> such as the argdumper, lldb-server and such... It's not fully clear to
> me whether the extra resolving magic is actually desirable here, but I
> couldn't see a reason why we shouldn't have it, so I kept the status
> quo.