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

Allow using pydevd as a regular dependency. #902

Closed
wants to merge 1 commit into from

Conversation

Apteryks
Copy link

This makes it easier to test using PyDev-Debugger (pydevd) as a
standalone dependency rather than as a bundled (current status) one,
which comes with binaries checked in Git.

  • setup.py (DEBUGPY_BUNDLING_DISABLED): New variable.
    [DEBUGPY_BUNDLING_DISABLED]: Conditionalize 'debugpy._vendored'
    import.
    (PYDEVD_ROOT): Set only when DEBUGPY_BUNDLING_DISABLED is False.
    (ExtModules.bool): Make the return value the negation of
    DEBUGPY_BUNDLING_DISABLED.
    (main)[DEBUGPY_BUNDLING_DISABLED]: New condition to call
    'cython_build'. Etch a bundling_disabled variable on the debugpy
    module.
    <setuptools.setup>: Do not include the _vendored submodule in the
    packages and package_data when DEBUGPY_BUNDLING_DISABLED is set.
    Adjust has_ext_modules argument.
  • src/debugpy/init.py: New 'bundling_disabled' variable.
  • src/debugpy/server/init.py [debugpy.bundling_disabled]:
    Conditionalize 'debugpy._vendored.force_pydevd' import.
  • src/debugpy/server/attach_pid_injected.py (attach): Import the
    attach_script procedure from sys.path directly when
    debugpy.bundling_disabled is true and do not do any sys.path
    manipulation in this case.
  • tests/tests/test_vendoring.py: Skip test if
    'debugpy.bundling_disabled' is true.
  • src/debugpy/server/init.py: Partially copy the pydevd loading
    logic from src/debugpy/_vendored/force_pydevd.py.

This should make it easier to test using PyDev-Debugger (pydevd) as a
standalone dependency rather than as a bundled (current status) one,
which comes with binaries checked in Git.  This in turn should make
solving issue 179 limited in scope to PyDev-Debugger itself.

* setup.py (DEBUGPY_BUNDLING_DISABLED): New variable.
[DEBUGPY_BUNDLING_DISABLED]: Conditionalize 'debugpy._vendored'
import.
(PYDEVD_ROOT): Set only when DEBUGPY_BUNDLING_DISABLED is False.
(ExtModules.__bool__): Make the return value the negation of
DEBUGPY_BUNDLING_DISABLED.
(__main__)[DEBUGPY_BUNDLING_DISABLED]: New condition to call
'cython_build'.  Etch a __bundling_disabled__ variable on the debugpy
module.
<setuptools.setup>: Do not include the _vendored submodule in the
packages and package_data when DEBUGPY_BUNDLING_DISABLED is set.
Adjust has_ext_modules argument.
* src/debugpy/__init__.py: New '__bundling_disabled__' variable.
* src/debugpy/server/__init__.py [debugpy.__bundling_disabled__]:
Conditionalize 'debugpy._vendored.force_pydevd' import.
* src/debugpy/server/attach_pid_injected.py (attach): Import the
attach_script procedure from sys.path directly when
debugpy.__bundling_disabled__ is true and do not do any sys.path
manipulation in this case.
* tests/tests/test_vendoring.py: Skip test if
'debugpy.__bundling_disabled__' is true.
* src/debugpy/server/__init__.py: Partially copy the pydevd loading
logic from src/debugpy/_vendored/force_pydevd.py.
@ghost
Copy link

ghost commented Apr 12, 2022

CLA assistant check
Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

❌ Apteryks sign now
You have signed the CLA already but the status is still pending? Let us recheck it.

@sonarcloud
Copy link

sonarcloud bot commented Apr 12, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@@ -11,19 +11,25 @@
import sys


DEBUGPY_BUNDLING_DISABLED = bool(os.getenv('DEBUGPY_BUNDLING_DISABLED'))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For clarity, let's invert the meaning - i.e. BUNDLE_DEBUGPY which defaults to true but can be overridden.

cython_build()

# Etch bundling status in the source.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is necessary - the code can check at runtime whether debugpy._vendored is present by trying to import it and catching ImportError. This can be cached as a package global to make one-liner checks easier.

Also, if caching, I'd prefer the variable name to not use the __reserved__ naming pattern - it can be simply is_pydevd_bundled or something like that. We declare our public API surface via __all__, so the name not starting with _ is not a concern.

from _pydevd_bundle import pydevd_defaults
pydevd_defaults.PydevdCustomization.DEFAULT_PROTOCOL = pydevd_constants.HTTP_JSON_PROTOCOL
else:
import debugpy._vendored.force_pydevd # noqa
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a lot of duplicated code. I think force_pydevd should be refactored to only contain the logic that is related to vendoring, while this one can do all the configuration instead, and conditionally call the vendoring part at the right point (there might be more than one, so force_pydevd might need to be split into functions).

Copy link
Contributor

@int19h int19h left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please also run it through Flake8 and Black.

@int19h
Copy link
Contributor

int19h commented May 23, 2022

Please re-open if this is still relevant.

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