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

Add jdwp support #61

Merged
merged 20 commits into from
Oct 25, 2023
Merged

Add jdwp support #61

merged 20 commits into from
Oct 25, 2023

Conversation

Daquiver1
Copy link
Contributor

This PR tackles issue #56.
It creates a module for the JDWP commands and errors over here

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Oct 11, 2023
Copy link
Contributor

@michalgr michalgr left a comment

Choose a reason for hiding this comment

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

Great start !

I have a suggestion to organize code a bit differently. I would:

  • move Type, Field, Struct, Command and Command set to projects/jdwp/defs/schema.py
  • move all remaining enums to projects/jdwp/defs/constants.py
  • move definitions of command sets to projects/jdwp/defs/commandsets/...

jdwp/common.py Outdated Show resolved Hide resolved
jdwp/common.py Outdated Show resolved Hide resolved
jdwp/common.py Outdated Show resolved Hide resolved
Copy link
Contributor

@michalgr michalgr left a comment

Choose a reason for hiding this comment

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

Not all commands are correctly described. In particular, some of them should have array fields and this PR does not add support for Arrays. Let's extract a part of this PR that can be landed sooner and will unblock @wekesa360.

Could you please do the following:

  • address feedback in schema.py and constants/*.py
  • move all definitions of constants to defs/constants.py
  • for now remove
    • defs/commands/thread_reference.py and defs/command_sets/thread_reference.py
    • all commands fromdefs/commands/reference_type.py but the Signature command
  • move definition of Signature from defs/commands/reference_type.py to defs/command_sets/reference_type.py
  • make ReferenceType command set incomplete, consisting of only one command (Signature) for now
  • rebase on top of main and make sure code builds with buck and checks with pyre.

projects/jdwp/defs/schema.py Outdated Show resolved Hide resolved
projects/jdwp/defs/schema.py Outdated Show resolved Hide resolved
projects/jdwp/defs/commands/reference_type.py Outdated Show resolved Hide resolved
projects/jdwp/defs/schema.py Outdated Show resolved Hide resolved
projects/jdwp/defs/schema.py Outdated Show resolved Hide resolved
projects/jdwp/defs/schema.py Outdated Show resolved Hide resolved
projects/jdwp/defs/schema.py Outdated Show resolved Hide resolved
.vscode/settings.json Show resolved Hide resolved
projects/jdwp/constants/errors.py Outdated Show resolved Hide resolved
projects/jdwp/constants/invoke_options.py Outdated Show resolved Hide resolved
@michalgr
Copy link
Contributor

Could you also start files with this copyright ?
https://github.com/facebookexperimental/ExtendedAndroidTools/blob/main/projects/jdwp/main.py#L1

@michalgr
Copy link
Contributor

Almost perfect!

Pyre does not type check new files because they are not part of any buck target. Could you add
projects/jdwp/defs/BUCK file with the following content ?

# Copyright (c) Meta Platforms, Inc. and affiliates.

python_library(
    name = "defs",
    srcs = glob(["**/*.py"]),
    visibility = ["PUBLIC"],
)

This will make all the new files visible to buck and pyre.

projects/jdwp/defs/schema.py Outdated Show resolved Hide resolved
projects/jdwp/defs/command_sets/reference_type.py Outdated Show resolved Hide resolved
projects/jdwp/defs/command_sets/reference_type.py Outdated Show resolved Hide resolved
@@ -8,7 +8,7 @@ python_binary(


python_library(
name = "lib",
srcs = [],
name = "defs",
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not projects/jdwp/defs/BUCK, but that is fine. In this case please keep the original name ("lib") and we should be good to go.

@michalgr michalgr merged commit 198aeb2 into facebookexperimental:main Oct 25, 2023
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants