search

hyperledger / aries-cloudagent-python

Hyperledger Aries Cloud Agent Python (ACA-Py) is a foundation for building decentralized identity applications and services running in non-mobile environments.
https://wiki.hyperledger.org/display/aries
Apache License 2.0
405 stars 510 forks source link

issue #3182: replace deprecated ptvsd debugger by debugpy #3183

Closed rngadam closed 1 month ago

rngadam commented 1 month ago

issue #3182

keeping ptvsd environment variables for backward compatibility

swcurran commented 1 month ago

Thanks for the update. Can you please search the documentation (*.md files) for references to ptvsd, and update changes in those as needed. Also, are there any places in the dockerfiles that need to be updated? Might not, but I have a faint memory there were some things there.

rngadam commented 1 month ago

Thanks for the update. Can you please search the documentation (*.md files) for references to ptvsd, and update changes in those as needed. Also, are there any places in the dockerfiles that need to be updated? Might not, but I have a faint memory there were some things there.

ENABLE_PTVSD, PTVSD_HOST, PTVSD_PORT should keep working as is, but with debugpy as the underlying implementation. Is that ok? If that's acceptable, then we don't need to change wherever those environment variables are used. Otherwise yes, there are a couple places where we'd need to rename PTVSD for DEBUGPY.

There are two places I ought to update in any cases:

swcurran commented 1 month ago

I think it is OK to keep the existing config parameters, although in retrospect it is unfortunate that they weren’t just “DEBUG” in the first place. Who knew the implementation would change :-). Suggest at least updating the config parameters information to note that debugpy is being used instead.

OK — one more question. Is it easy in argparse to have two names for the same config? If that were possible we could have both the PTVSD and DEBUG for the config parameters. Not a big deal...

Thanks!

rngadam commented 1 month ago

I think it is OK to keep the existing config parameters, although in retrospect it is unfortunate that they weren’t just “DEBUG” in the first place. Who knew the implementation would change :-). Suggest at least updating the config parameters information to note that debugpy is being used instead.

both PTVSD and debugpy are implementation of DAP. The reason it is not debug I think is because there is an alternative to debug with Pycharm too.

OK — one more question. Is it easy in argparse to have two names for the same config? If that were possible we could have both the PTVSD and DEBUG for the config parameters. Not a big deal...

I added DAP_HOST and DAP_PORT as alternates environment variables. Note that this is done in main, not argparse before anything else starts.

Updated doc and conftest.py (although I'm not sure how to test the latter?).

rngadam commented 1 month ago

Also, seems like the reason for ENABLE_PTVSD was to get around the fact that some of the modes of the agent would reject the --debug flag. Now that it is in its own group Debugger, all modes can be supported.

swcurran commented 1 month ago

Thanks — I think that looks good, but leave to the dev maintainers to weigh in — @jamshale @dbluhm @amanji @ianco .

Please address the file conflict — required before merging — which looks like is also causing the synk issue.

sonarcloud[bot] commented 1 month ago

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
94.7% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

rngadam commented 1 month ago

Please address the file conflict — required before merging — which looks like is also causing the synk issue.

done. BTW seems like poetry content-hash merge conflict is a long-standing issue:

https://github.com/python-poetry/poetry/issues/496