Add a VSCode devcontainer definition #916

GeorgeLyon · 2023-10-20T20:11:16Z

I'm not sure if this is interesting, but when putting together this PR I did my development in a devcontainer (instructions provided in a Readme). As I'm often managing many different environments and tools (and VSCode extensions), I really like the devcontainer workflow and on M-series Macs the performance is quite good.

GeorgeLyon · 2023-10-20T23:51:04Z

One thing that I wasn't able to figure out is if we can add the -Xcxx -I/usr/lib/swift -Xcxx -I/usr/lib/swift/Block arguments somewhere global in the devcontainer, which I believe would enable using VSCode's test explorer with this configuration which would be very nice.

adam-fowler · 2023-10-21T11:25:25Z

@GeorgeLyon Do you know about the Add Dev Container Configuration Files... command?
If you

select From a predefined container configuration ...
select Show All Definitions...
Type swift into the filter box
Select the Swift devcontainer
You get the recommended swift container. You can find the template here https://github.com/swift-server/swift-devcontainer-template
There are also a number of features you can add, including both swift formats, sqlite, foundation networking and jemalloc

When you talk about VSCode's test explorer which test explorer are you talking about? There are multiple test explorers.

GeorgeLyon · 2023-10-21T14:31:28Z

Thanks for the info! This was adapted from a slightly more complicated devcontainer I had on another project, but knowing there is a default Swift template is nice. In my other project I also used the devcontainers/ci action to run CI on the devcontainer which I really like.

I haven't used features much, but I opted to use the Dockerfile to install most things to keep things simple and more standard (I'm pretty sure Features are a devcontainer thing not a Docker thing). That said... I think they provide some interesting functionality and would be worth looking into.

One not on the Swift devcontainer template... I also really like using zsh in my devcontainers, but I do it in a slightly-bad way using a dotfiles repo and the "dotfiles.repository": VSCode setting. This way I can separate "things actually related to the project" and "things George likes" (the "default devcontainer extensions" option helps with this too)

GeorgeLyon · 2023-10-21T14:36:16Z

When you talk about VSCode's test explorer which test explorer are you talking about? There are multiple test explorers.

I'm talking about the default "beaker" one in the UI. I gather sourcekit-lsp opts in to some "I detect tests" API which populates it. Unfortunately, because you need to specify extra commandline flags for the sourcekit-lsp project (-Xcxx -I/usr/lib/swift..., which I could find no way to specify for running from the test explorer), the tests fail to build correctly.

Perhaps the right thing to do here would be to make a package-config file named something like "swiftToolchain" that adds these flags. Might give it a shot later today.

adam-fowler · 2023-10-21T14:43:57Z

That Test Explorer is generated by the swift extension. Go into the settings and you can add additional build arguments in the Swift: Build Arguments section. Make sure you only do it for the current workspace.

As an alternative you can add a tasks.json entry with command line parameters. Press Cmd+Shift+B and click on cog symbol at right side of menu entry for the build you want to edit.

Both of these options create files in the .vscode folder a settings.json or a tasks.json respectively. Perhaps one of these should be committed to the source kit-lsp repo

GeorgeLyon · 2023-10-22T18:37:10Z

Go into the settings and you can add additional build arguments in the Swift: Build Arguments section
Oh man, I missed this because I only checked the sections nested under "Swift" in the extensions UI. I can confirm this works, and I added this to the devcontainer "settings" section, which writes it to the "Remote Settings" section in the devcontainer. Now the Test Explorer UI works out-of-the-box in the devcontainer, which is pretty cool.

Not sure this is the best approach as it is workspace-specific. The approach I've been taking in my own projects is to bind a different directory at .vscode in the devcontainer. This works pretty well if you commit to not having per-project VSCode settings, tasks or launch jsons (though you can still achieve this with some symlinking). I like this approach because I almost always need to tweak some setting in different devcontainer environments.

GeorgeLyon · 2023-10-22T18:43:20Z

Also all that being said, I think the pkg-config approach is actually the "correct" way to handle these sorts of arguments. Setting this for the whole project won't work as the path is different depending on what system you are on and what toolchain you are using. Conceptually, you are depending on a library that is installed on the system, much the same way you would depend on something like sqlite3 if that was installed on the system. In a devcontainer this would work well since you likely would never need more than one Swift toolchain installed, and could just create a pkg-config for "the Swift toolchain installed on my system".
A more robust solution would be for the toolchain itself to provide a "this toolchain" .systemLibrary (or even a new enum, .toolchainLibrary or something like that) which resolves correctly even in an environment where you have multiple toolchains installed. Not something I have time to chase down at the moment, so just leaving my thoughts here.

ahoppen · 2023-10-31T21:04:26Z

Sorry for taking a while to look at this. I think having a devcontainer is an exciting idea and it should simplify getting started with sourcekit-lsp development, especially on Linux where the setup is non-trivial.

I am not sure if I followed the entire conversation but are there any open questions left? With regards to the Dockerfile, I would prefer if it follows the default template because that way we have one standardized Dockerfile setting for new packages adding a container and sourcekit-lsp.

GeorgeLyon · 2023-10-31T21:47:00Z

With regards to the Dockerfile, I would prefer if it follows the default template

Not sure what you mean by "follows the default template". The swift template @adam-fowler was referring to is a template for adding these files (devcontainer.json, etc) to a Swift project which doesn't have them. It directly references a Docker image by name, which is insufficient for developing sourcekit-lsp. My Dockerfile uses an official image as the base of the devcontainer (the FROM command) and then installs the dependencies of sourcekit-lsp on top of that (the RUN command).

One way to think about it is the template is a way to get a reasonable starting point when adding a new devcontainer to a project that doesn't have one, and this PR is a fleshed-out devcontainer that actually works for sourcekit-lsp.

ahoppen · 2023-10-31T22:43:22Z

Oh, sorry. I misunderstood you there. From the discussion I thought that Add Dev Container Configuration Files… added a Dockerfile. Ignore my comment then about the Dockerfile then.

Are there any open questions/tasks on this PR or is this ready for review as far as you are concerned @GeorgeLyon? Just trying to make sure I’m not commenting on something that’s not fully ready yet.

GeorgeLyon · 2023-10-31T22:48:46Z

Not from my end, I just have really liked the devcontainer experience recently and this is just the setup I used to dabble in this repo, so if it can be helpful to someone else, great!

ahoppen · 2023-10-31T23:02:39Z

Definitely! I agree, having a standardized installation setup is great. I’d like to test the devcontainer myself but it might take me a while until I get to it. I’ll come back to to this PR once I tried it.

ahoppen · 2024-05-21T21:29:01Z

@GeorgeLyon I am really sorry how long it took us to get back to you on this PR. I wanted to push this forward for the last few months and was only now able to set up an environment in which I could test the dev container. Would you still be interested in continuing the PR? If so, could you re-open it? If you don’t have time to work on it anymore, I understand that as well.

GeorgeLyon · 2024-05-21T21:35:36Z

Happy to keep it open, I was just cleaning up my "open PRs" list .

ahoppen · 2024-05-21T22:11:08Z

Sure, I totally understand. Don’t blame you for assuming that there was no interest on our side.

.devcontainer/default/Dockerfile

.devcontainer/default/devcontainer.json

ahoppen · 2024-05-21T22:15:46Z

.devcontainer/default/devcontainer.json

+    "vscode": {
+      "extensions": [
+        "sswg.swift-lang",
+        "vknabel.vscode-apple-swift-format"


Similar to my comment above, swift-format should be run using the Run swift-format action, so this shouldn’t be needed anymore.

This extension uses swift-format from the project if it is a dependency. I like including it because it makes other VSCode functionality (like format-on-save) work correctly.

You can get format on save to work using SourceKit-LSP in Swift 6.0. The vscode extension shouldn't be needed anymore.

You are correct! Sorry I didn't realize this was added.

Sources/SKTestSupport/INPUTS/.gitignore

ahoppen · 2024-05-21T22:23:14Z

.devcontainer/Readme.md

+### Setup
+
+1. Install Docker, a tool for managing containerized VMs: https://www.docker.com
+  - If you have Docker installed, make sure it is updated to the most recent version (there is a "Check for Updates" option in the application UI).
+  - If installing Docker on macOS for the first time, I recommend selecting the "Advanced" installation option, specifying a "User" installation and disabling the two  options below. This makes it so your Docker installation does not require root privileges for anything, which is generally nice and makes updates more seamless. This will require you telling VSCode where the `docker` executable ended up by changing the "dev.containers.dockerPath" setting (usually to something like `"/Users/<your-user-name>/.docker/bin/docker”`). You should make sure this executable exists by executing `"/<expected-path-to-docker>/docker --version”`.
+2. Install Visual Studio Code and the Remote Containers extensions: https://code.visualstudio.com/docs/devcontainers/tutorial
+3. Configure Docker by opening up the Docker application and navigating to "Settings"
+  - Recommended settings for macOS (some of these are defaults):
+    - General:
+      - "Choose file sharing implementation for your containers": VirtioFS (better IO performance)
+    - Resources:
+      - CPUs: Allow docker to use most or all of your CPUs
+      - Memory: Allow docker to use most or all of your memory
+4. Open up this repository in VSCode
+  - VSCode has an "Install 'code' command in PATH" command which installs a helpful tool to open VSCode from the commandline (`code <path-to-this-repo>`)
+5. Select "Reopen in Container" in the notification that pops up
+  - If there is no popup you can manually open the dev container by selecting "Dev Containers: Rebuild and Reopen in Container" from the command palette (Command-Shift-P on macOS)
+  - Occasionally, after pulling from git, VSCode may prompt you to rebuild the container if the container definition has changed
+6. Wait for the container to be built


I would prefer linking to a tutorial (or similar) instead of duplicating information about how to set up VS Code with dev containers in this repository because such documentation always has the tendency to get out-of-date. What do you think about linking to https://code.visualstudio.com/docs/devcontainers/tutorial?

I think the only specific things not mentioned in the tutorial would be the following:

- Recommended settings for macOS (some of these are defaults): - General: - "Choose file sharing implementation for your containers": VirtioFS (better IO performance) - Resources: - CPUs: Allow docker to use most or all of your CPUs - Memory: Allow docker to use most or all of your memory

Links are also sometimes broken, which is why I both linked to it and wrote up the short version (this was actually just copy-pasta from another repo of mine). Happy to just use your copy, but I don't the current copy will be problematic.

My preference would be to just reference the tutorial and just add the SourceKit-LSP-specific details here. My opinions is that we shouldn’t try to re-do work that somebody else has already done.

ahoppen · 2024-05-22T18:49:41Z

.devcontainer/default/Dockerfile

+ENV \
+  PATH="/swift-format/.build/debug:${PATH}"


This also shouldn’t be needed anymore if we don’t install swift-format, right?

ahoppen · 2024-05-22T18:52:05Z

.devcontainer/default/devcontainer.json

+
+  "mounts": [
+    // Use a named volume for the build products for optimal performance (https://code.visualstudio.com/remote/advancedcontainers/improve-performance?WT.mc_id=javascript-14373-yolasors#_use-a-targeted-named-volume)
+    "source=${localWorkspaceFolderBasename}-build,target=${containerWorkspaceFolder}/build,type=volume"


Shouldn’t this be using .build instead of build? Otherwise I don’t think we get anything out of it.

Suggested change

"source=${localWorkspaceFolderBasename}-build,target=${containerWorkspaceFolder}/build,type=volume"

"source=${localWorkspaceFolderBasename}-build,target=${containerWorkspaceFolder}/build,type=volume"

And since you’re at it, I think it makes sense to also add a named volume to the .index-build that’s used for experimental background indexing.

Yes, good catch!

ahoppen · 2024-05-22T19:49:18Z

.devcontainer/Readme.md

+### Setup
+
+1. Install Docker, a tool for managing containerized VMs: https://www.docker.com
+  - If you have Docker installed, make sure it is updated to the most recent version (there is a "Check for Updates" option in the application UI).
+  - If installing Docker on macOS for the first time, I recommend selecting the "Advanced" installation option, specifying a "User" installation and disabling the two  options below. This makes it so your Docker installation does not require root privileges for anything, which is generally nice and makes updates more seamless. This will require you telling VSCode where the `docker` executable ended up by changing the "dev.containers.dockerPath" setting (usually to something like `"/Users/<your-user-name>/.docker/bin/docker”`). You should make sure this executable exists by executing `"/<expected-path-to-docker>/docker --version”`.
+2. Install Visual Studio Code and the Remote Containers extensions: https://code.visualstudio.com/docs/devcontainers/tutorial
+3. Configure Docker by opening up the Docker application and navigating to "Settings"
+  - Recommended settings for macOS (some of these are defaults):
+    - General:
+      - "Choose file sharing implementation for your containers": VirtioFS (better IO performance)
+    - Resources:
+      - CPUs: Allow docker to use most or all of your CPUs
+      - Memory: Allow docker to use most or all of your memory
+4. Open up this repository in VSCode
+  - VSCode has an "Install 'code' command in PATH" command which installs a helpful tool to open VSCode from the commandline (`code <path-to-this-repo>`)
+5. Select "Reopen in Container" in the notification that pops up
+  - If there is no popup you can manually open the dev container by selecting "Dev Containers: Rebuild and Reopen in Container" from the command palette (Command-Shift-P on macOS)
+  - Occasionally, after pulling from git, VSCode may prompt you to rebuild the container if the container definition has changed
+6. Wait for the container to be built


My preference would be to just reference the tutorial and just add the SourceKit-LSP-specific details here. My opinions is that we shouldn’t try to re-do work that somebody else has already done.

ahoppen · 2024-05-22T19:49:29Z

.devcontainer/default/devcontainer.json

@@ -0,0 +1,31 @@
+// Reference: https://containers.dev/implementors/json_reference/
+{
+  "name": "SourceKit LSP",


Nitpick

Suggested change

"name": "SourceKit LSP",

"name": "SourceKit-LSP",

ahoppen

Nice. I have three more minor comments, otherwise, let’s get this shipped 🚢

.devcontainer/default/devcontainer.json

.devcontainer/default/Dockerfile

Co-authored-by: Alex Hoppen <[email protected]>

adam-fowler

Some comments, based on what I'm using

adam-fowler · 2024-05-23T15:19:17Z

.devcontainer/default/Dockerfile

@@ -0,0 +1,10 @@
+FROM swiftlang/swift:nightly-main-jammy


Should this be 6.0? @ahoppen what are your thoughts?

Suggested change

FROM swiftlang/swift:nightly-main-jammy

FROM swiftlang/swift:nightly-6.0-jammy

I don't have strong thoughts, but when I put this together I believe the guidance was that I should be using a "recent nightly". This should follow whatever the guidance is currently.

@ahoppen can make the call. I guess if it is for development maybe it should stay at main

We should use the latest main because the main branch of SourceKit-LSP might be using features from sourcekitd, that are only available in main. When running SourceKit-LSP tests with an older version of sourcekitd, those tests would be skipped.

adam-fowler · 2024-05-23T15:28:36Z

.devcontainer/default/devcontainer.json

+        "sswg.swift-lang"
+      ],
+      "settings": {
+        "lldb.launch.expressions": "native",


You don't need to do this. The swift extension does it for you

I don't know if this has changed, but when the extension does it for you it prompts you where to store the setting, with one of the option (I believe the default) being "Global" and the other being "Workspace". Both will conflict with the setting needed for opening the workspace locally on macOS, and setting it here has it end up in the "remote" settings which is the correct place for it. If this setting is not what the extension expect I believe it will still prompt you to change it.

The extension prompts you but it'll have already prompted you outside of the devcontainer.

Let’s remove it then. Fewer settings is always good.

OK, for the record I'm not removing the setting below because it actually needs to be different inside the container vs out.

Yes the setting below is required

adam-fowler · 2024-05-23T15:31:32Z

.devcontainer/default/devcontainer.json

+{
+  "name": "SourceKit-LSP",
+  "dockerFile": "Dockerfile",
+


Is it worthwhile using the common-utils feature to setup a vscode user so we aren't running as root eg

"features": { "ghcr.io/devcontainers/features/common-utils:2": { "installZsh": "false", "username": "vscode", "userUid": "1000", "userGid": "1000", "upgradePackages": "false" }, "ghcr.io/devcontainers/features/git:1": { "version": "os-provided", "ppa": "false" }, }, "remoteUser": "vscode"

I also include the git feature although I'm not certain that is necessary

Happy to add this but just curious what the downsides are of running as root? I'd also want to make sure that this is indeed something that is relevant to the project, and not just something the user can set in their own dev.containers.defaultFeatures setting. I also don't know how the project setting interacts with a user setting (i.e. if I have installZsh = true in my defaults and it is false here what happens?)

I guess it's a security issue. All of Microsoft's language images have a vscode user. And they use the common-utils feature to set it up. So there shouldn't be any issues. I guess in theory you shouldn't need the git feature as git is already in the swift image.

I would prefer to keep the .devcontainer.json file minimal. You will be running everything as root if you’re using the swift:nightly-main-jammy docker image, so I don’t think there’s any inherent harm in also running as root inside the devcontainer.

I'm not an expert, but AFAICT the "not running as root" stuff is more to do with software in the container making bad assumptions and not running properly as root than anything to do with security.

ahoppen

Wohoooo. Let’s get this in 🎉

ahoppen · 2024-05-23T20:03:41Z

@swift-ci Please test

GeorgeLyon requested review from benlangmuir and ahoppen as code owners October 20, 2023 20:11

GeorgeLyon closed this May 13, 2024

GeorgeLyon reopened this May 21, 2024

ahoppen reviewed May 21, 2024

View reviewed changes

ahoppen mentioned this pull request May 21, 2024

Add devcontainer for building SK-LSP #1308

Closed

GeorgeLyon added 3 commits May 21, 2024 15:48

Add devcontainer

33168ec

improve PATH

8581620

Add swift build arguments to devcontainer.json

4019986

GeorgeLyon force-pushed the dev/george/devcontainer branch from 4fa705d to f1090c4 Compare May 21, 2024 23:03

Respond to feedback

4ae907a

GeorgeLyon force-pushed the dev/george/devcontainer branch from f1090c4 to 4ae907a Compare May 21, 2024 23:06

lokesh-tr added a commit to lokesh-tr/sourcekit-lsp that referenced this pull request May 22, 2024

Initialise devcontainer based on swiftlang#916

ad6e97f

Remove extension

345095a

ahoppen reviewed May 22, 2024

View reviewed changes

Address feedback

6b4cf6f

ahoppen reviewed May 22, 2024

View reviewed changes

.devcontainer/default/devcontainer.json Outdated Show resolved Hide resolved

.devcontainer/default/devcontainer.json Outdated Show resolved Hide resolved

.devcontainer/default/Dockerfile Outdated Show resolved Hide resolved

GeorgeLyon and others added 3 commits May 22, 2024 19:13

Update .devcontainer/default/Dockerfile

6de819c

Co-authored-by: Alex Hoppen <[email protected]>

Update .devcontainer/default/devcontainer.json

e3c9fde

Co-authored-by: Alex Hoppen <[email protected]>

Update .devcontainer/default/devcontainer.json

cd07e2a

Co-authored-by: Alex Hoppen <[email protected]>

adam-fowler reviewed May 23, 2024

View reviewed changes

Remove lldb setting

c80437b

ahoppen approved these changes May 23, 2024

View reviewed changes

ahoppen enabled auto-merge (squash) May 23, 2024 20:05

ahoppen merged commit ba7b03c into swiftlang:main May 23, 2024
3 checks passed

	"source=${localWorkspaceFolderBasename}-build,target=${containerWorkspaceFolder}/build,type=volume"
	"source=${localWorkspaceFolderBasename}-build,target=${containerWorkspaceFolder}/build,type=volume"

	FROM swiftlang/swift:nightly-main-jammy
	FROM swiftlang/swift:nightly-6.0-jammy

		ENV \
		PATH="/swift-format/.build/debug:${PATH}"

Add a VSCode devcontainer definition #916

Add a VSCode devcontainer definition #916

Uh oh!

Conversation

GeorgeLyon commented Oct 20, 2023

Uh oh!

GeorgeLyon commented Oct 20, 2023

Uh oh!

adam-fowler commented Oct 21, 2023

Uh oh!

GeorgeLyon commented Oct 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

GeorgeLyon commented Oct 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adam-fowler commented Oct 21, 2023

Uh oh!

GeorgeLyon commented Oct 22, 2023

Uh oh!

GeorgeLyon commented Oct 22, 2023

Uh oh!

ahoppen commented Oct 31, 2023

Uh oh!

GeorgeLyon commented Oct 31, 2023

Uh oh!

ahoppen commented Oct 31, 2023

Uh oh!

GeorgeLyon commented Oct 31, 2023

Uh oh!

ahoppen commented Oct 31, 2023

Uh oh!

ahoppen commented May 21, 2024

Uh oh!

GeorgeLyon commented May 21, 2024

Uh oh!

ahoppen commented May 21, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahoppen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

adam-fowler left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

GeorgeLyon commented Oct 21, 2023 •

edited

Loading

GeorgeLyon commented Oct 21, 2023 •

edited

Loading

adam-fowler May 23, 2024 •

edited

Loading