Skip to content

chore(helios-sci-11): Devcontainer Changes + Hash Bump#15

Open
mhambre wants to merge 3 commits intomainfrom
helios-sci-11
Open

chore(helios-sci-11): Devcontainer Changes + Hash Bump#15
mhambre wants to merge 3 commits intomainfrom
helios-sci-11

Conversation

@mhambre
Copy link
Owner

@mhambre mhambre commented Mar 12, 2026

  • Mounts git creds/config to devcontainer
  • Cleans up Dockerfile to remove old i686 downloads
  • Fixes rust-analyzer/clippy not working on save

@mhambre mhambre requested a review from Copilot March 12, 2026 03:29
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Updates the devcontainer setup for Helios SCI to improve developer experience (Git/SSH integration, Rust tooling behavior) and removes legacy i686-specific Rust setup, alongside a submodule hash bump.

Changes:

  • Bump crates/helios-sci submodule pointer.
  • Update devcontainer mounts/ports and rust-analyzer configuration for better on-save checks.
  • Simplify devcontainer Dockerfile and Rust toolchain setup (remove i686, add QEMU packages).

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.

File Description
crates/helios-sci Updates submodule commit hash.
.devcontainer/post-create.sh Switches Rust nightly install from i686 toolchain/target to generic nightly + x86_64 target/components.
.devcontainer/devcontainer.json Adds cargo cache volumes + host Git/SSH bind mounts, forwards GDB port, tweaks rust-analyzer settings.
.devcontainer/Dockerfile Removes redundant apt commands, adds QEMU packages, simplifies nightly component install/targets.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

Comment on lines +14 to +17
"source=${localEnv:HOME}/.gitconfig,target=/home/vscode/.gitconfig,type=bind,consistency=cached",
"source=${localEnv:HOME}/.ssh/config,target=/home/vscode/.ssh/config,type=bind,consistency=cached,readonly",
"source=${localEnv:HOME}/.ssh/known_hosts,target=/home/vscode/.ssh/known_hosts,type=bind,consistency=cached,readonly",
"source=${localEnv:HOME}/.ssh/keys,target=/home/vscode/.ssh/keys,type=bind,consistency=cached,readonly"
Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

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

Binding host SSH key material into the container (~/.ssh/keys) significantly increases the blast radius if the container is compromised (keys become readable inside the container). Prefer SSH-agent forwarding (bind-mount SSH_AUTH_SOCK and set SSH_AUTH_SOCK in the container) so private keys never enter the container filesystem. Also consider making the .gitconfig mount read-only to prevent accidental modification of host config from inside the container.

Copilot uses AI. Check for mistakes.
Copy link
Owner Author

@mhambre mhambre Mar 12, 2026

Choose a reason for hiding this comment

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

@copilot This seems like more of a concern for a public container, no? As this is a devcontainer we have no intention of ever hosting anything from it. If that's still problematic I can bind mount SSH_AUTH_SOCK.

Comment on lines +16 to +17
"source=${localEnv:HOME}/.ssh/known_hosts,target=/home/vscode/.ssh/known_hosts,type=bind,consistency=cached,readonly",
"source=${localEnv:HOME}/.ssh/keys,target=/home/vscode/.ssh/keys,type=bind,consistency=cached,readonly"
Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

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

Docker bind mounts fail container startup if the source path does not exist. ~/.gitconfig, ~/.ssh/config, ~/.ssh/known_hosts, and especially ~/.ssh/keys are not guaranteed to exist on all developer machines, so this can make the devcontainer non-bootable for new setups. A more robust approach is to mount a directory that is guaranteed to exist (or create the needed files in an initializeCommand before the container starts), or document the required host-file prerequisites in the devcontainer README.

Suggested change
"source=${localEnv:HOME}/.ssh/known_hosts,target=/home/vscode/.ssh/known_hosts,type=bind,consistency=cached,readonly",
"source=${localEnv:HOME}/.ssh/keys,target=/home/vscode/.ssh/keys,type=bind,consistency=cached,readonly"
"source=${localEnv:HOME}/.ssh/known_hosts,target=/home/vscode/.ssh/known_hosts,type=bind,consistency=cached,readonly"

Copilot uses AI. Check for mistakes.
Comment on lines 32 to +37
RUN rustup target add x86_64-unknown-linux-gnu || true

# Install rustfmt and clippy for code formatting and linting
# Install nightly components used by low-level crate builds
RUN rustup toolchain install nightly || true \
&& rustup component add clippy --toolchain nightly || true \
&& rustup component add --toolchain nightly-x86_64-unknown-linux-gnu rustfmt rust-src || true \
&& cargo fetch || true
&& rustup component add --toolchain nightly clippy rustfmt rust-src || true \
&& rustup target add --toolchain nightly x86_64-unknown-linux-gnu || true
Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

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

Using || true here masks failures during image build (e.g., network issues, rustup server errors), which can leave the container in a partially configured state and make later failures harder to diagnose (e.g., rust-analyzer/clippy missing). Prefer failing the build if rustup commands fail, or gate these steps explicitly (e.g., check whether the toolchain/components are already installed and only skip when truly unnecessary).

Copilot uses AI. Check for mistakes.
],
"settings": {
"editor.formatOnSave": true,
"rust-analyzer.checkOnSave": true,
Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

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

The VS Code rust-analyzer settings schema typically uses rust-analyzer.check.* (as you already do) and/or rust-analyzer.checkOnSave.* subkeys; a boolean rust-analyzer.checkOnSave may be ignored by the extension and can be confusing to future maintainers. Consider removing it if unnecessary (since check.command drives what runs on save) or switching to the supported checkOnSave configuration shape for enabling/disabling on-save checks.

Suggested change
"rust-analyzer.checkOnSave": true,

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants