Page MenuHomePhabricator

Teach GetXcodeSDK to look in the Xcode that contains LLDB

Authored by aprantl on Jun 4 2020, 7:07 PM.



Teach GetXcodeSDK to look in the Xcode that contains LLDB instead of preferring the one chosen with xcode-select. Because we're using xcrun to find matching SDK's you can now get into a situation where LLDB, when run from a non-xcode-selected Xcode will find a matching SDK in the xcode-selected Xcode, which can cause anything from mild performance degradation to really confusing Clang compile errors, if the other Xcode is, for example, older, or missing an SDK.


Diff Detail

Event Timeline

friss added a comment.Jun 5 2020, 9:03 AM

This LGTM, with the caveat that it doesn't handle command line tools. But we didn't handle it correctly before, so not something we need to fix here.

JDevlieghere requested changes to this revision.Jun 5 2020, 9:37 AM
JDevlieghere added inline comments.

Above we use getenv, here we use Host::GetEnvironment. We should pick one and be consistent.


It looks like this is basically dealing with one case (shlib) of the logic in GetXcodeContentsDirectory. I would prefer to make this a separate function GetXcodeDeveloperDirectory or something that does just that.

Before I refactored these functions there were several methods where we tried to share code like this. While I'm generally a strong opponent of sharing code, this lead to the pattern we see here where we need to add complexity to the "generic" function to make things work. This in turn makes things more complex and harder to understand, up till the point that we inevitable implemented something "similar but different" elsewhere.

This revision now requires changes to proceed.Jun 5 2020, 9:37 AM
JDevlieghere added inline comments.Jun 5 2020, 9:40 AM

Not something you should fix for this patch, but RunShellCommand should be able to take environment variables.

aprantl updated this revision to Diff 268870.Jun 5 2020, 10:27 AM

Address feedback from Jonas!

JDevlieghere added inline comments.Jun 5 2020, 10:38 AM

Any reason you choose to leave this inline and not making it a separate function? If we did we could also cache the result like we do for some of the other paths.

aprantl marked 2 inline comments as done.Jun 5 2020, 10:42 AM
aprantl added inline comments.

I will create a follow-up patch for above.


This function is going to be called at most ~7*2 times, once for each XcodeSDK kind * Internal. So caching isn't really relevant. i also would like to refactor the other function after this patch, which would make factoring this out first really awkward.

JDevlieghere accepted this revision.Jun 5 2020, 11:01 AM



Sounds good!

This revision is now accepted and ready to land.Jun 5 2020, 11:01 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJun 5 2020, 12:26 PM