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 VSCode devcontainer config #3083

Merged
merged 4 commits into from
Mar 15, 2023
Merged

Conversation

GeorgeLyon
Copy link
Contributor

@GeorgeLyon GeorgeLyon commented Mar 12, 2023

Contributor Checklist

  • Did you add Scaladoc to every public function/method?
  • Did you add at least one test demonstrating the PR?
  • Did you delete any extraneous printlns/debugging code?
  • Did you specify the type of improvement?
  • Did you add appropriate documentation in docs/src?
  • Did you state the API impact?
  • Did you specify the code generation impact?
  • Did you request a desired merge strategy?
  • Did you add text to be included in the Release Notes for this change?

Type of Improvement

Development ease-of-use improvement.

API Impact

None

Backend Code Generation Impact

None

Desired Merge Strategy

Squash

Release Notes

Added standard config for VSCode dev containers that can get developers up and running with just Docker and VSCode. To use, simply open the repo in VSCode and choose "Rebuild and Reopen in Container".

Reviewer Checklist (only modified by reviewer)

  • Did you add the appropriate labels?
  • Did you mark the proper milestone (Bug fix: 3.5.x or 3.6.x depending on impact, API modification or big change: 5.0.0)?
  • Did you review?
  • Did you check whether all relevant Contributor checkboxes have been checked?
  • Did you do one of the following when ready to merge:
    • Squash: You/ the contributor Enable auto-merge (squash), clean up the commit message, and label with Please Merge.
    • Merge: Ensure that contributor has cleaned up their commit history, then merge with Create a merge commit.

Comment on lines +25 to +32
# Install GraalVM
# This downloads all relevant GraalVM architectures at once, mostly because $TARGETARCH values don't map exactly to the release URLs. Since we're optimizing for developer experience here and not image size, this is OK
# GraalVM release links can be found here: https://github.com/graalvm/graalvm-ce-builds/releases
ADD https://github.com/graalvm/graalvm-ce-builds/releases/download/vm-22.3.1/graalvm-ce-java17-linux-aarch64-22.3.1.tar.gz /graalvm/tarballs/arm64.tar.gz
ADD https://github.com/graalvm/graalvm-ce-builds/releases/download/vm-22.3.1/graalvm-ce-java17-linux-amd64-22.3.1.tar.gz /graalvm/tarballs/amd64.tar.gz
RUN tar -xzf /graalvm/tarballs/$TARGETARCH.tar.gz -C /graalvm --strip-components=1
ENV JAVA_HOME=/graalvm
ENV PATH=$JAVA_HOME/bin/:$PATH
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need both architectures? I thought the point of Docker was that you can run everything as if it were running on the Linux in the container.

Copy link
Contributor Author

@GeorgeLyon GeorgeLyon Mar 15, 2023

Choose a reason for hiding this comment

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

You can run, for instance, x86 containers on arm64 machines (or at least, on Apple Silicon). The main downside to doing so is performance (since this probably works by running in something like QEMU), so it's always preferable to have your container be the same architecture as your host system (which is achieved via the --platform flag in the FROM command).

Copy link
Contributor

Choose a reason for hiding this comment

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

I see, nice!

@sequencer
Copy link
Member

I personally using nix as the development environment, which provides a better support to multi-arch, if using devcontainer, there is also a support for which. WDYT

@GeorgeLyon
Copy link
Contributor Author

GeorgeLyon commented Mar 15, 2023

I haven't great experiences with nix-in-docker and VSCode (I'm actually exploring this for a different Codebase I'm involved with, and have played with the repo you linked). If you can get it to work, that is great! My gut feeling though is that it will be more complicated and have more dependencies (for example on the person maintaining nix-devcontainer).
That said local nix could also be something supported by Chisel. I just find Nix's macOS installation flow a bit too invasive for my tastes (last I checked it involved creating a special volume, among other things).

@sequencer
Copy link
Member

If you can get it to work, that is great!

IIRC, I did some black magic on mill bsp. I will check that after back to my office.

Copy link
Contributor

@jackkoenig jackkoenig left a comment

Choose a reason for hiding this comment

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

nix debate aside, I think it's reasonable to merge this.

I think it would also be cool if we could evolve this into a Docker container for CI and for Chisel users as well. In any case, this is useful as is.

@jackkoenig jackkoenig enabled auto-merge (squash) March 15, 2023 01:39
@jackkoenig jackkoenig added the Please Merge Accepted PRs that are ready to be merged. Useful when waiting on CI. label Mar 15, 2023
@GeorgeLyon
Copy link
Contributor Author

Any clues on what the merge failure is about? It says something about me not being a user. I have already contributed to Chisel, so not sure what is happening.

@jackkoenig
Copy link
Contributor

jackkoenig commented Mar 15, 2023

Any clues on what the merge failure is about? It says something about me not being a user. I have already contributed to Chisel, so not sure what is happening.

I think there must be a Mergify bug. There is a Premium Plan Feature to impersonate Github users when updating pull requests, but I'm pretty sure we're just using the free open-source edition of Mergify and it uses its own account to update PRs in this repo (eg. see #3056 (comment)). If we were able to and using that feature, then the docs say "The user account must have already been
logged in Mergify dashboard once and have write or maintain permission.", so it makes sense it would fail for you, but obviously this shouldn't apply.

Anyway, I'll just update the branch manually--we also should consider not requiring PRs to be up-to-date. I wish we could require PRs to be "up-to-date-ish", like CI has been run in the last 3 days or something.

@jackkoenig
Copy link
Contributor

Github has merge queues now, maybe that's a better answer to reduce CI churn while still testing that PRs are up-to-date.

@jackkoenig jackkoenig merged commit bfb1f2d into chipsalliance:main Mar 15, 2023
@GeorgeLyon GeorgeLyon deleted the devcontainer branch March 15, 2023 05:21
@GeorgeLyon
Copy link
Contributor Author

Thanks! GitHub merge queues sound interesting, you know I’m all about fewer dependencies :)

@jackkoenig jackkoenig added the Internal Internal change, does not affect users, will be included in release notes label Apr 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Internal Internal change, does not affect users, will be included in release notes Please Merge Accepted PRs that are ready to be merged. Useful when waiting on CI.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants