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 --debug-adapter flag to run #15829

Merged
merged 23 commits into from
Jun 27, 2022
Merged

Add --debug-adapter flag to run #15829

merged 23 commits into from
Jun 27, 2022

Conversation

thejcannon
Copy link
Member

@thejcannon thejcannon commented Jun 15, 2022

run's counterpart to #15799. Support is added for pex_binary (which will be spiritually lifted to the python_source with #15849)

[ci skip-rust]
[ci skip-build-wheels]

# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
@thejcannon thejcannon requested review from kaos, stuhood and Eric-Arellano and removed request for stuhood June 15, 2022 00:30
# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
Copy link
Member

@kaos kaos left a comment

Choose a reason for hiding this comment

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

Nice incremental introduction.

Copy link
Contributor

@Eric-Arellano Eric-Arellano left a comment

Choose a reason for hiding this comment

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

I don't recommend landing the public option until it can be used for something. That's confusing for users why they would ever use it.

What's the motivation to land this now?

@thejcannon
Copy link
Member Author

I suppose I can hold off until https://docs.google.com/document/d/1R9m-DtM8Y5UKx5iam5ipeK531tZR40_P7El4ucYq4Po is in

@thejcannon thejcannon marked this pull request as draft June 15, 2022 20:19
@thejcannon
Copy link
Member Author

I've pushed some code which gets--debug-adapter working for module-only entry-points which aren't run in sandbox. It has a few rough edges so holding off for now.

# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
@thejcannon thejcannon marked this pull request as ready for review June 24, 2022 15:32
# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
@thejcannon
Copy link
Member Author

Not a draft anymore thanks to debugpy.main_spec_args.

I'll make a follow-up PR to integrate that into the test goal, as well as move the options to a new subsystem so its more global.

Also, I'm struggling to find a reasonable way to automated-test these, which we very much should. Ideas welcome!

# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
@thejcannon
Copy link
Member Author

I'm going to wait until #15928 is in and use it here.
Additionally, I'll split the main_spec_args into a dedicated PR just to play nice.

@stuhood
Copy link
Member

stuhood commented Jun 24, 2022

Also: does this need any doc updates? Perhaps to the python run page?

@thejcannon
Copy link
Member Author

Also: does this need any doc updates? Perhaps to the python run page?

Yeah I owe docs for run and test for --debug-adapter.

# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
@thejcannon thejcannon added this to the 2.13.x milestone Jun 27, 2022
# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
Copy link
Contributor

@Eric-Arellano Eric-Arellano left a comment

Choose a reason for hiding this comment

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

THanks! All my feedback applies to the test docs too

docs/markdown/Python/python-goals/python-run-goal.md Outdated Show resolved Hide resolved
docs/markdown/Python/python-goals/python-test-goal.md Outdated Show resolved Hide resolved
docs/markdown/Python/python-goals/python-run-goal.md Outdated Show resolved Hide resolved
docs/markdown/Python/python-goals/python-run-goal.md Outdated Show resolved Hide resolved
docs/markdown/Python/python-goals/python-run-goal.md Outdated Show resolved Hide resolved
src/python/pants/backend/python/goals/run_pex_binary.py Outdated Show resolved Hide resolved
ResolvePexEntryPointRequest(field_set.entry_point),
),
Get(RunRequest, PexBinaryFieldSet, field_set),
Get(Pex, PexRequest, debugpy.to_pex_request()),
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we avoid installing debugpy if you aren't using it? That saves time and avoids possible issues with the PEX not being installable.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is in a RunDebugAdapterRequest rule, so when would we not be using it?

# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
Copy link
Contributor

@Eric-Arellano Eric-Arellano left a comment

Choose a reason for hiding this comment

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

Thanks! Looks good other than grammar

src/python/pants/backend/python/goals/run_pex_binary.py Outdated Show resolved Hide resolved
> using the `[debug-adapter]` subsystem).
> 1. In your editor, set your breakpoints and any other debug settings (like break-on-exception).
> 2. Run your code with `./pants run --debug-adapter`.
> 3. Connect your editor to the server. The server host and port are logged by Pants when executing `run --debug-adaptor`. (They can also be configured using the `[debug-adapter]` subsystem).
Copy link
Contributor

Choose a reason for hiding this comment

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

How do you connect to the server?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's largely editor specific 🤷‍♂️

Copy link
Contributor

Choose a reason for hiding this comment

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

With VSCode? I'm wondering if we should link to a certain guide, for example.

Copy link
Member Author

Choose a reason for hiding this comment

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

For instance, in VS Code for Python you set up a "Python: Remote Attach" launch.json configuration and with the default values, you start it (F5).

# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
softwrap(
f"""
Using `run --debug-adapter` on the target {field_set.address}, which sets the field
`run_in_sandbox` set to `True`. This will likely cause your breakpoints to not be hit,
Copy link
Contributor

Choose a reason for hiding this comment

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

You still say "set" twice.

Sorry, I normally wouldn't be this picky if it were a comment. But because it's documentation, I think it matters more.

Copy link
Member Author

Choose a reason for hiding this comment

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

No need to apologize, I wanna get this right as well. Sorry my self-review-goggles are fuzzy today,

# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
@thejcannon thejcannon merged commit bf25e12 into pantsbuild:main Jun 27, 2022
@thejcannon thejcannon deleted the dap2 branch June 27, 2022 21:36
thejcannon added a commit to thejcannon/pants that referenced this pull request Jun 28, 2022
run's counterpart to pantsbuild#15799. Support is added for pex_binary (which will be spiritually lifted to the python_source with pantsbuild#15849)

[ci skip-rust]
[ci skip-build-wheels]
thejcannon added a commit to thejcannon/pants that referenced this pull request Jun 28, 2022
run's counterpart to pantsbuild#15799. Support is added for pex_binary (which will be spiritually lifted to the python_source with pantsbuild#15849)

[ci skip-rust]
[ci skip-build-wheels]
stuhood pushed a commit that referenced this pull request Jun 29, 2022
`run`'s counterpart to #15799. Support is added for `pex_binary` (which will be spiritually lifted to the `python_source` with #15849)

[ci skip-rust]
[ci skip-build-wheels]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants