Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Makes the listen process configurable. #387

Merged
merged 3 commits into from
Sep 21, 2020

Conversation

bob-white
Copy link
Contributor

Basic idea for making the listen process configurable for applications
that embed python, but aren't python processes. Can help with #334

Basic idea for making the listen process configurable for applications
that embed python, but aren't python processes.
@int19h
Copy link
Contributor

int19h commented Aug 25, 2020

We already have a config property for this - it's "python" (with "pythonPath" as legacy spelling), it's just that it's currently only used for launch configurations, but we should repurpose that for attach. It also needs a test.

Also, per #262 (comment), a useful heuristic is to probe sys.exec_prefix.

@bob-white
Copy link
Contributor Author

Interesting, didn't see the other config, and from looking at the listen code, it would get rejected if I tried to use it with debugpy.configure as it doesn't pre-exist in the _config dictionary.

So python would be a better spelling than process?

Also for reference in our world editor:

>>> sys.exec_prefix
''

So not sure what exactly we'd get out of that in this case.

@int19h
Copy link
Contributor

int19h commented Aug 25, 2020

The stuff in _config is properties that must be set using debugpy.configure() when setting up for attach (i.e. that can't go into launch.json or equivalent on client side, because by the time the debugger sees that, it's already too late). So "python" isn't there, because it doesn't have any meaning for attach currently. But since it already exists for launch, and its meaning there is to specify the path to Python interpreter used to perform the launch, I think it makes sense to repurpose it for this.

As far as exec_prefix - if I understand @dalmago correctly, it is set for Blender and Maya, so for those cases having it would mean that users don't have to configure anything manually.

@bob-white
Copy link
Contributor Author

I think they'd still need to configure something with sys.exec_prefix for example Maya doesn't ship with a python executable, instead it has maya and mayapy. 3dsMax and Houdini would have similar problems as they also ship with standalone python interpreters that aren't named python. (3dsMaxpy and hython respectively.)

Blender seems to be the oddly well behaved one out of the bunch because for it, os.path.join(sys.exec_prefix, 'python') actually works.

So something almost like this would need to be done.

    adapter_python = _config.get('python', sys.executable)
    if not os.path.exists(adapter_python):
        adapter_python = os.path.join(sys.exec_prefix, adapter_python)

This sadly doesn't work if you're just saying "use the python executable I have on PATH"
So maybe attempt to launch the adapter a few times with variations of the input until one works or they all fail?

@int19h
Copy link
Contributor

int19h commented Aug 26, 2020

If "python" is specified explicitly, it should just be used with no modification, without trying to exists-check it.

But for the cases where it's not specified, the default could be sys.executable if it exists, otherwise exec_prefix. Although I'm not sure that would also work reliably, since sys.executable isn't necessarily missing - it's just not Python. Let's keep it simple and stick to sys.executable for now, then.

@bob-white
Copy link
Contributor Author

Okay, that feels better to me as well.
The thing with sys.exec_prefix is you'd still need to feed it something to complete the path, otherwise its just a folder that the python process probably lives in.

So one other question, when it comes to adding a test for this, what is the best way to go about that? I've never worked with pytest before, and I couldn't find any other tests that were working debugpy.configure to use as a starting point.

@int19h
Copy link
Contributor

int19h commented Aug 26, 2020

I've just realized that this would be very tricky to test, because it's a process used by the debug server to spawn the debug adapter, and the communication between the two is an implementation detail from the perspective of our (black box) tests.

So, to observe the difference, the test would need to provide a wrapper around the actual Python interpreter that just registers the fact that it was invoked. Which is doable in principle - but it can't be an actual binary for the tests to remain platform-independent. We could probably rig something reasonably portable up with cython --embed, but it's definitely not simple.

I'll think some more about it, but we can do the tests in another PR, so as not to block this one.

@int19h
Copy link
Contributor

int19h commented Aug 31, 2020

Is the PR ready for merging, or are you planning on adding any more commits?

@sonarcloud
Copy link

sonarcloud bot commented Sep 21, 2020

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants