This is an archive of the discontinued LLVM Phabricator instance.

Differential D135577

Summary: This documentation patch adds information to allow remote users to also use the plugin as it will be invisible to them using the current instructions. It solves issue #58252.
ClosedPublic

Authored by HenriqueBucher on Oct 10 2022, 6:34 AM.

Details

Reviewers
clayborg
JDevlieghere
Commits
rG5a52c5c42669: Summary: This documentation patch adds information to allow remote users to…
Summary

This documentation patch adds information to allow remote users to also use the plugin as it will be invisible to them using the current instructions. It solves issue #58252.

Diff Detail

Event Timeline

HenriqueBucher created this revision.Oct 10 2022, 6:34 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 10 2022, 6:34 AM
HenriqueBucher requested review of this revision.Oct 10 2022, 6:34 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 10 2022, 6:34 AM
Herald added a subscriber: lldb-commits. · View Herald Transcript
Harbormaster completed remote builds in B191262: Diff 466484.Oct 10 2022, 6:38 AM
Herald added a subscriber: JDevlieghere. · View Herald TranscriptOct 10 2022, 6:38 AM
JDevlieghere requested changes to this revision.Oct 10 2022, 9:09 AM
JDevlieghere added inline comments.
lldb/tools/lldb-vscode/README.md
39

Please wrap this so it fits in the 80 col limit.

70–71

Remove

This revision now requires changes to proceed.Oct 10 2022, 9:09 AM
jryans added a subscriber: jryans.Oct 10 2022, 9:38 AM

Please also give this patch a title that would make sense as the first line of a commit message. It currently just says "Summary:" at the moment.

HenriqueBucher updated this revision to Diff 466537.Oct 10 2022, 9:48 AM

Formatted text to wrap in 80 columns

HenriqueBucher updated this revision to Diff 466538.Oct 10 2022, 9:49 AM

Removed 2 extra blank lines

HenriqueBucher added inline comments.Oct 10 2022, 9:51 AM
lldb/tools/lldb-vscode/README.md
39

Most of this file has lines with more than 80 columns. Would you want the other lines to be reformatted as well?

Harbormaster completed remote builds in B191307: Diff 466538.Oct 10 2022, 9:52 AM
JDevlieghere added inline comments.Oct 10 2022, 10:22 AM
lldb/tools/lldb-vscode/README.md
39

I wouldn't say "most": there are a few lines that exceed the 80 col limit for a valid reason (i.e. having a URL in them) but you're right that there's a few lines that should be reflowed (line 25 for example). Since that would be orthogonal to this patch, it should be a separate patch. For the same reason there's no expectation for you to do that, but it would of course be very much appreciated!

HenriqueBucher added a comment.Oct 10 2022, 1:19 PM

Ok submitted another patch. Added you as reviewer. Check if it is okay.
https://reviews.llvm.org/D135607

clayborg added a comment.Oct 10 2022, 5:32 PM

lgtm. Jonas?

JDevlieghere accepted this revision.Oct 11 2022, 9:36 AM

LGTM

This revision is now accepted and ready to land.Oct 11 2022, 9:36 AM
JDevlieghere added a comment.Oct 11 2022, 9:38 AM

Henrique, do you have commit access?

Whoever ends up committing this, please make sure the commit has a reasonable commit description and not "Summary:".

HenriqueBucher added a comment.Oct 11 2022, 7:52 PM

I assume not since I have never committed on any LLVM project.
But I would like to as I plan to contribute more in the future.
Just sent Chris Clattner an email with the request.
Note: The reason the summary is so terse is that the ARC tool opens two windows and I was very confused about where to write what.
Maybe a little paragraph in the documentation would make this clearer?

DavidSpickett added a subscriber: DavidSpickett.Oct 12 2022, 12:38 AM

Note: The reason the summary is so terse is that the ARC tool opens two windows and I was very confused about where to write what.

Certainly would confuse me, maybe I've learned to autopilot through this though. Can you describe them? We can certainly add a note in the docs.

DavidSpickett added a comment.Oct 12 2022, 12:39 AM

Oh and if you would like someone to land this on your behalf please provide a name and email address for the commit (of course waiting for access is fine too).

HenriqueBucher updated this revision to Diff 467310.Oct 12 2022, 5:23 PM

Visual Studio code has different folders for plugins when used by local users and users remoting into the box.
The current instructions show how to install the lldb-vscode plugin only for the local users and it will be invisible to them if they remote.
This patch adds instructions on how to install the plugin for remote users too.
It solves issue #58252.

HenriqueBucher updated this revision to Diff 467313.Oct 12 2022, 5:26 PM
HenriqueBucher retitled this revision from Summary: to Summary: This documentation patch adds information to allow remote users to also use the plugin as it will be invisible to them using the current instructions. It solves issue #58252..

The current documentation shows how to install the lldb-vscode plugin as a local user.
When they remote, the plugin will be invisible.
This documentation patch adds information on how to extend the plugin to remote users as well.

Harbormaster completed remote builds in B191858: Diff 467313.Oct 12 2022, 5:29 PM
Closed by commit rG5a52c5c42669: Summary: This documentation patch adds information to allow remote users to… (authored by HenriqueBucher). · Explain WhyOct 12 2022, 5:29 PM
This revision was automatically updated to reflect the committed changes.
HenriqueBucher added a commit: rG5a52c5c42669: Summary: This documentation patch adds information to allow remote users to….
HenriqueBucher added a comment.Oct 12 2022, 5:32 PM

Got a very nice email from Chris Lattner with the approval. Very nice.
Dude I took the plunge and did it. I hope it went through right. Please let me know if I screwed it up.

jryans added a comment.Oct 12 2022, 5:48 PM

Exciting, welcome to the LLVM community! 🙂

Overall your commit looks okay, but here are a few tips for the future... The "Summary:" prefix in the commit title is not needed. It's good to start the commit title with an active verb e.g. "Add foo to bar". You can optionally use prefix tags like "[Docs]" to suggest the area. I recommend looking over other commit messages (https://github.com/llvm/llvm-project/commits/main) to get a feel for the typical style.

HenriqueBucher added a comment.Oct 13 2022, 11:48 AM

Is there anything to be done at this point? Can I reset my branch?

jryans added a comment.Oct 13 2022, 3:28 PM

No, once it has been committed, it can’t really be removed from a project of this size. Changes can be reverted, but the existence of the commits and messages remains. Don’t worry about it for this one, just something to think about next time.

HenriqueBucher added a comment.Oct 13 2022, 4:13 PM

I mean my local changes, can I consider this done and clean up?
Sorry for the very newbie question.

jryans added a comment.Oct 13 2022, 4:17 PM

Ah yes, it’s all done now, feel free to clean up your local state however you prefer.

Revision Contents

PathSize
lldb/
tools/
lldb-vscode/
8 lines

Diff 467314

lldb/tools/lldb-vscode/README.md

Show All 30 Lines
``` ```
$ mkdir -p ~/.vscode/extensions/llvm-org.lldb-vscode-0.1.0/bin $ mkdir -p ~/.vscode/extensions/llvm-org.lldb-vscode-0.1.0/bin
$ cp package.json ~/.vscode/extensions/llvm-org.lldb-vscode-0.1.0 $ cp package.json ~/.vscode/extensions/llvm-org.lldb-vscode-0.1.0
$ cd ~/.vscode/extensions/llvm-org.lldb-vscode-0.1.0/bin $ cd ~/.vscode/extensions/llvm-org.lldb-vscode-0.1.0/bin
$ cp /path/to/a/built/lldb-vscode . $ cp /path/to/a/built/lldb-vscode .
$ cp /path/to/a/built/liblldb.so . $ cp /path/to/a/built/liblldb.so .
``` ```
It is important to note that the directory `~/.vscode/extensions` works for users logged in locally to the machine. If you are remoting into the box using Visual Studio Code's Remote plugins (SSH, WSL, Docker) it will look for extensions on `~/.vscode-server/extensions` only and you will not see your just installed lldb-vscode plug-in. If you want this plugin to be visible to remoting users, you will need to either repeat the process above for the `~/.vscode-server` folder or create a symbolic link from it to `~/.vscode/extensions`:
JDevlieghereUnsubmitted
Not Done
ReplyInline Actions

Please wrap this so it fits in the 80 col limit.

JDevlieghere: Please wrap this so it fits in the 80 col limit.
HenriqueBucherAuthorUnsubmitted
Done
ReplyInline Actions

Most of this file has lines with more than 80 columns. Would you want the other lines to be reformatted as well?

HenriqueBucher: Most of this file has lines with more than 80 columns. Would you want the other lines to be…
JDevlieghereUnsubmitted
Not Done
ReplyInline Actions

I wouldn't say "most": there are a few lines that exceed the 80 col limit for a valid reason (i.e. having a URL in them) but you're right that there's a few lines that should be reflowed (line 25 for example). Since that would be orthogonal to this patch, it should be a separate patch. For the same reason there's no expectation for you to do that, but it would of course be very much appreciated!

JDevlieghere: I wouldn't say "most": there are a few lines that exceed the 80 col limit for a valid reason (i.
```
$ cd ~/.vscode-server/extensions
$ ln -s ~/.vscode/extensions/llvm-org.lldb-vscode-0.1.0 llvm-org.lldb-vscode-0.1.0
```
If you want to make a stand alone plug-in that you can send to others on macOS systems: If you want to make a stand alone plug-in that you can send to others on macOS systems:
``` ```
$ mkdir -p ~/.vscode/extensions/llvm-org.lldb-vscode-0.1.0/bin $ mkdir -p ~/.vscode/extensions/llvm-org.lldb-vscode-0.1.0/bin
$ cp package.json ~/.vscode/extensions/llvm-org.lldb-vscode-0.1.0 $ cp package.json ~/.vscode/extensions/llvm-org.lldb-vscode-0.1.0
$ cd ~/.vscode/extensions/llvm-org.lldb-vscode-0.1.0/bin $ cd ~/.vscode/extensions/llvm-org.lldb-vscode-0.1.0/bin
$ cp /path/to/a/built/lldb-vscode . $ cp /path/to/a/built/lldb-vscode .
Show All 9 Lines
$ mkdir -p ~/.vscode/extensions/llvm-org.lldb-vscode-0.1.0/bin $ mkdir -p ~/.vscode/extensions/llvm-org.lldb-vscode-0.1.0/bin
$ cp package.json ~/.vscode/extensions/llvm-org.lldb-vscode-0.1.0 $ cp package.json ~/.vscode/extensions/llvm-org.lldb-vscode-0.1.0
$ cd ~/.vscode/extensions/llvm-org.lldb-vscode-0.1.0/bin $ cd ~/.vscode/extensions/llvm-org.lldb-vscode-0.1.0/bin
$ ln -s /path/to/a/built/lldb-vscode $ ln -s /path/to/a/built/lldb-vscode
``` ```
This is handy if you want to debug and develope the `lldb-vscode` executable when adding features or fixing bugs. This is handy if you want to debug and develope the `lldb-vscode` executable when adding features or fixing bugs.
JDevlieghereUnsubmitted
Not Done
ReplyInline Actions

Remove

JDevlieghere: Remove
# Configurations # Configurations
Launching to attaching require you to create a [launch configuration](https://code.visualstudio.com/Docs/editor/debugging#_launch-configurations). This file Launching to attaching require you to create a [launch configuration](https://code.visualstudio.com/Docs/editor/debugging#_launch-configurations). This file
defines arguments that get passed to `lldb-vscode` and the configuration settings defines arguments that get passed to `lldb-vscode` and the configuration settings
control how the launch or attach happens. control how the launch or attach happens.
## Launch Configuration Settings ## Launch Configuration Settings
▲ Show 20 Lines • Show All 126 Lines • Show Last 20 Lines