Replace outdated unified build example with a devcontainer which demonstrates a unified build #6305

GeorgeLyon · 2023-10-18T21:23:55Z

The old example wasn't used as far as I can tell. The added benefit of this approach is that we can validate the configuration in CI as well.

This can also be used "out-of-the-box" (provided you have VSCode devcontainers set up) as opposed to having to copy and modify the example.`

dtzSiFive

Wow. Helping VSCode users is good but don't know about this. Will try to find time to revisit later. Thanks for submitting/sharing.

dtzSiFive · 2023-10-19T13:00:50Z

.devcontainer/cmake-helper

+
+# Calls CMake, loading arguments from devcontainer settings to maintain a single source of truth.
+
+PROJECT_NAME=circel


Suggested change

PROJECT_NAME=circel

PROJECT_NAME=circt

Or, what's circel?

Sorry, thought I had gotten all the copypasta... This whole infra is something I was using for SIL, and am currently using a revised version of for Chisel bindings (circel).

dtzSiFive · 2023-10-19T13:03:30Z

.devcontainer/default/Dockerfile

+
+# Different versions of clang and Java can be selected by specifying the following build arguments, but are not guaranteed to work.
+ARG CLANG_VERSION=17
+ARG JAVA_VERSION=java11


copy pasta, will remove

dtzSiFive · 2023-10-19T13:04:44Z

.devcontainer/default/VSCode/settings.json

+    "-DLLVM_USE_SANITIZER=",
+
+    // LLVM (performance)
+    "-DLLVM_USE_LINKER=lld-17",


is lld-17 good here, or should this be set/substituted in? Similarly, clangd-17 below.

Oh generally looks like this hardcodes things that are variables elsewhere (e.g., build paths). Should this be generated instead?

Yeah, the Dockerfile essentially defines the state of the VM. In this VM, all the LLVM stuff is suffixed 17 (because that is how the package is set up). A different VM would have different hardcodings. I wouldn't generate this, as it is nice to not need to run a "generator" before being able to use the project. The least I would do is validate this is accurate in CI, which this PR kind of does.

dtzSiFive · 2023-10-19T13:05:46Z

.devcontainer/default/VSCode/settings.json

+    "--compile-commands-dir=${workspaceFolder}/build/circt",
+    "--header-insertion=never",
+    "--clang-tidy",
+    "--clang-tidy-checks=*",


Just curious, why all checks? (and not using the project's .clang-tidy?)

The clang-tidy stuff is relatively new, I'll remove this line

dtzSiFive · 2023-10-19T13:06:18Z

.devcontainer/default/devcontainer.json

+  "runArgs": [
+    "--cap-add=SYS_PTRACE",
+    "--security-opt",
+    "seccomp=unconfined"


What's this about?

dtzSiFive · 2023-10-19T13:43:14Z

.devcontainer/default/Dockerfile

+      jq \
+      #
+      # ShellCheck is useful for validating bash scripts
+      shellcheck \


dtzSiFive · 2023-10-19T13:45:44Z

.devcontainer/Readme.md

+
+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, making updates more seamless. This will require you telling VSCode where the `docker` executable ended up by chaging the "dev.containers.dockerPath" setting `"/Users/<your-user-name>/.docker/bin/docker”`. You should make sure this executable exists by executing `"/Users/<your-user-name>/.docker/bin/docker --version”`, if you are not on macOS the `docker` executable might be at a different path.


Suggested change

- 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, making updates more seamless. This will require you telling VSCode where the `docker` executable ended up by chaging the "dev.containers.dockerPath" setting `"/Users/<your-user-name>/.docker/bin/docker”`. You should make sure this executable exists by executing `"/Users/<your-user-name>/.docker/bin/docker --version”`, if you are not on macOS the `docker` executable might be at a different path.

- 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, making updates more seamless. This will require you telling VSCode where the `docker` executable ended up by chamging the "dev.containers.dockerPath" setting `"/Users/<your-user-name>/.docker/bin/docker”`. You should make sure this executable exists by executing `"/Users/<your-user-name>/.docker/bin/docker --version”`, if you are not on macOS the `docker` executable might be at a different path.

dtzSiFive · 2023-10-19T13:46:01Z

.devcontainer/Readme.md

@@ -0,0 +1,40 @@
+## Using VSCode devcontainers
+
+VSCode supports creating development environments inside of a docker container. With some light configuration, we can get a consistent development environment that is robust to changes in your local environment and also doesn't require you to install and manage dependencies on your machine.


cries in Nix

I've tried to use nix every so often. The fact that it nix hyper-intrusive on Mac (haven't checked if this has changed recently) is non-starter for me. Docker can be installed with no privileges and uses Apple's VM frameworks so is quite performant on M-series chips. I find I don't actually need Nix's level of hermeticity, and Docker's usability wins for me here.

dtzSiFive · 2023-10-19T13:46:45Z

.devcontainer/cmake-helper

+      fi
+    fi
+
+    set -e


Unless I missed something, this is set at top already and not unset?

dtzSiFive · 2023-10-19T13:48:48Z

.devcontainer/cmake-helper

@@ -0,0 +1,94 @@
+#!/usr/bin/env bash
+
+set -e


Consider, set -euo pipefail -- or however you want to do it-- make error using variables not set (looks like nice and careful about this so might as well) and make a | b | c not only fail if c does.

Good idea, I keep meaning to do this but only set -e is stored in my brain.

GeorgeLyon · 2023-10-19T14:22:57Z

@teqdruid Could be interesting to unify this with the CI image (i.e. CI can just be an additional devcontainer configuration).

mikeurbach

Thanks for providing an updated setup, I think it is good to have things like this checked in. However, I'm also concerned about the impact on CI from this change, both in terms of cache and runtime. It looked like the validate devcontainer step took a significant amount of CI time on this PR. IMHO it's good to have local setup things like checked in, but I don't know if it's worth the trade off to have them checked by CI.

mikeurbach · 2023-10-19T17:38:28Z

.github/workflows/buildAndTest.yml

@@ -73,6 +72,10 @@ jobs:
        run: |
          # Run clang-format
          git clang-format $DIFF_COMMIT
+
+          # clang-format unnecessarily formats some JSON in this directory, and disagrees with VSCode's formatting rules
+          git checkout HEAD -- .devcontainer


Is there a way to configure clang-format to ignore certain files?

Not that I found. Pretty weird that it formats JSON too...

mikeurbach · 2023-10-19T17:42:26Z

.github/workflows/validateDevcontainer.yml

+            ./.devcontainer/cmake-helper configure
+
+      # We run the build, once to check the targets and once to log the errors without any progress logs cluttering the output (since it is a trivial incremental build). Please take care to make sure the following two steps stay in sync.
+      - name: Check targets


Do we need to add a full run of the tests? At least on this PR, it seems to be a long pole on the pre merge CI.

GeorgeLyon · 2023-10-25T22:28:01Z

@mikeurbach I tweaked some things and now the devcontainer validates in just over 5 minutes on a cache hit, which seems pretty reasonable. Cache size isn't that bad either, coming in at under 400MB.

GeorgeLyon requested a review from mikeurbach October 18, 2023 21:23

GeorgeLyon requested review from teqdruid and dtzSiFive as code owners October 18, 2023 21:23

dtzSiFive reviewed Oct 19, 2023

View reviewed changes

mikeurbach reviewed Oct 25, 2023

View reviewed changes

GeorgeLyon added 16 commits October 27, 2023 11:25

Add docs

45565a0

Remove Metals-related settings

f020b09

Add missing file and don't format VSCode JSONs

61c06ce

Copy cmake-helper as well

56c472a

Try _clang-format

3668289

Manually ignore clang-format things

91734fe

Also ignore devcontainer in clang-tidy

355b443

Remove Configure Unified Build step

37ddf55

Address feedback

f2c02af

Remove unused compiler selection logic from cmake-helper

6a6bca5

Make more things project-independent

92f5df8

Don't run tests

9fbf1ba

Build only firtool for validation

9f44710

Test incremental build

2224cc6

Tweak cache key logic

41a5cc9

Test cache

3317678

GeorgeLyon force-pushed the dev/george/devcontainer branch from d4aa19b to 3317678 Compare October 27, 2023 18:25

GeorgeLyon closed this Mar 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace outdated unified build example with a devcontainer which demonstrates a unified build #6305

Replace outdated unified build example with a devcontainer which demonstrates a unified build #6305

GeorgeLyon commented Oct 18, 2023

dtzSiFive left a comment

dtzSiFive Oct 19, 2023

GeorgeLyon Oct 19, 2023

dtzSiFive Oct 19, 2023

GeorgeLyon Oct 19, 2023

dtzSiFive Oct 19, 2023

dtzSiFive Oct 19, 2023

GeorgeLyon Oct 19, 2023

dtzSiFive Oct 19, 2023

GeorgeLyon Oct 19, 2023

dtzSiFive Oct 19, 2023

dtzSiFive Oct 19, 2023

dtzSiFive Oct 19, 2023

dtzSiFive Oct 19, 2023

GeorgeLyon Oct 19, 2023

dtzSiFive Oct 19, 2023

dtzSiFive Oct 19, 2023

GeorgeLyon Oct 19, 2023

GeorgeLyon commented Oct 19, 2023

mikeurbach left a comment

mikeurbach Oct 19, 2023

GeorgeLyon Oct 25, 2023

mikeurbach Oct 19, 2023

GeorgeLyon commented Oct 25, 2023


		# Calls CMake, loading arguments from devcontainer settings to maintain a single source of truth.

		PROJECT_NAME=circel

		@@ -0,0 +1,40 @@
		## Using VSCode devcontainers

		VSCode supports creating development environments inside of a docker container. With some light configuration, we can get a consistent development environment that is robust to changes in your local environment and also doesn't require you to install and manage dependencies on your machine.

+                    fi
+                  fi
+                  set -e

Replace outdated unified build example with a devcontainer which demonstrates a unified build #6305

Replace outdated unified build example with a devcontainer which demonstrates a unified build #6305

Conversation

GeorgeLyon commented Oct 18, 2023

dtzSiFive left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GeorgeLyon commented Oct 19, 2023

mikeurbach left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GeorgeLyon commented Oct 25, 2023