Page MenuHomePhabricator

Remove dependency from Host to python
ClosedPublic

Authored by labath on Jun 15 2018, 4:57 AM.

Details

Summary

The only reason python was used in the Host module was to compute the
python path. I resolve this the same way as D47384 did for clang, moving
the path computation into the python plugin and modifying SBHostOS class
to call into this module for ePathTypePythonDir.

Diff Detail

Repository
rL LLVM

Event Timeline

labath created this revision.Jun 15 2018, 4:57 AM
clayborg added inline comments.
source/API/SBHostOS.cpp
48 ↗(On Diff #151489)

Why is this here instead of inside of "HostInfo::GetLLDBPath(path_type, fspec);"? Does this mean internal code that calls:

HostInfo::GetLLDBPath(ePathTypePythonDir, fspec);

Won't work anymore? Also same for ePathTypeClangDir? Are we trying to solve a layering problems by hosing over internal clients?

zturner added a subscriber: labath.Jun 15 2018, 8:22 AM

The internal api has no guarantees as to its stability.

The internal api has no guarantees as to its stability.

What do you mean by this? If we have an internal API that claims to give out paths, it should give out paths. What am I missing here?

It can assert when we pass the python or clang directory enumeration. We
could also remove the values from the internal enumeration but keep them in
the public enumeration. However, we can’t be forced into providing a stable
internal api just because we must provide a stable public api.

Actually, my plan was to follow this up with a patch which removes usages of that enumeration from the internal api altogether (basically do a s/GetLLDBPath(ePathTypeXXX/GetXXXPath). That should make the internal api nicer, and provide a clean separation between the internal and public api (thereby taking pressure off of adding new enums to the public api just because we have a new kind of an internal path).

I was considering in which order to submit the patches. In the end I chose this one because it will make the second patch slightly smaller, but I can do it in the other order too...

(pressed return too soon)

Would that address your reservations about this idea?

It might be nice to start with the removal of HostInfo::GetLLDBPath(...) first and fixing up all call sites that use that API to use the new internally sanctioned versions. Then submit this patch?

(pressed return too soon)

Would that address your reservations about this idea?

yeah, just don't like the idea of HostInfo::GetLLDBPath() being non functional.

labath planned changes to this revision.Jun 15 2018, 9:14 AM

Ok, I'll start with the other patch first.

labath updated this revision to Diff 151922.Jun 19 2018, 8:27 AM

Rebase this patch on trunk. Now this patch is simply about moving GetPythonDir
from Host module to ScriptInterpreterPython.

clayborg accepted this revision.Jun 19 2018, 8:50 AM
This revision is now accepted and ready to land.Jun 19 2018, 8:50 AM
This revision was automatically updated to reflect the committed changes.