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

Initial version of a devcontainer for Verible #2068

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

kbrunham-intel
Copy link
Contributor

Initial version of the .devcontainer/ directory and configuration.

This devcontainer uses a Dockerfile based on the Microsoft CPP container, augmented to install Bazel, clang-format, and clang-tidy.

With the devcontainer, developers can clone the repo, and open it within a standard container definition.

@kbrunham-intel
Copy link
Contributor Author

@hzeller : Can you review and consider approving?

# Available versions: https://github.com/devcontainers/images/tree/main/src/cpp
FROM mcr.microsoft.com/devcontainers/cpp:ubuntu-22.04
RUN apt update && apt install -y clang-format clang-tidy
RUN wget https://github.com/bazelbuild/bazelisk/releases/download/v1.19.0/bazelisk-linux-amd64 -O /usr/local/bin/bazel && chmod a+rx /usr/local/bin/bazel && /usr/local/bin/bazel --version
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we fix bazel version to 5.x.x?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This specific code uses bazelisk to install the latest version of bazel. I didn't see the GitHub workflow for the repo doing something to obtain a 5.x.x. version, but may have missed it. Does the workflow do something specific?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok, using a .bazelversion file seems to be the way to go, I'll do that in a separate PR. FYI, the devcontainer will not work today without it since verible is not yet compatible with bazel 7.0.0

"name": "Verible Dev",
"build": {
"dockerfile": "Dockerfile",
// Add any proxy settings if the build of the
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please remove commented JSON about proxy settings

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've moved the details to the top of the file. Please comment if this meets the crux of your feedback.

@kbrunham-intel
Copy link
Contributor Author

@corco: Please let me know if there are any other changes needed.

@kbrunham-intel kbrunham-intel requested a review from corco January 13, 2024 21:39
@codecov-commenter
Copy link

codecov-commenter commented Jan 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (a4d61b1) 92.95% compared to head (4522d99) 92.95%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2068   +/-   ##
=======================================
  Coverage   92.95%   92.95%           
=======================================
  Files         358      358           
  Lines       26446    26446           
=======================================
  Hits        24583    24583           
  Misses       1863     1863           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

# Available versions: https://github.com/devcontainers/images/tree/main/src/cpp
FROM mcr.microsoft.com/devcontainers/cpp:ubuntu-22.04
RUN apt update && apt install -y clang-format clang-tidy
RUN wget https://github.com/bazelbuild/bazelisk/releases/download/v1.19.0/bazelisk-linux-amd64 -O /usr/local/bin/bazel && chmod a+rx /usr/local/bin/bazel && /usr/local/bin/bazel --version
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok, using a .bazelversion file seems to be the way to go, I'll do that in a separate PR. FYI, the devcontainer will not work today without it since verible is not yet compatible with bazel 7.0.0

@kbrunham-intel
Copy link
Contributor Author

Hi @corco : I don't have write access to the repo, so I can't merge this PR. Can you please merge it or grant me write access?

Thanks.

@corco corco closed this Jan 17, 2024
@corco corco reopened this Jan 17, 2024
@kbrunham-intel
Copy link
Contributor Author

@corco / @hzeller : Please merge as I don't have write access.

@hzeller
Copy link
Collaborator

hzeller commented Jan 19, 2024

Can you add a README to the directory that explains what this is for possibly with links pointing to documentation ?
It looks like a editor configuration for a specific (vscode ?); so for everyone else they then can see if this is relevant for them.

@corco
Copy link
Collaborator

corco commented Jan 31, 2024

Please add the environment variable USE_BAZEL_VERSION with value "6.5.0" in the devcontainer.json. Right now the devcontainer would use bazel 7.0 which is not able to compile verible. That environment variable should make it use a compatible bazel version

@hzeller
Copy link
Collaborator

hzeller commented Mar 7, 2024

Recent changes should make it possible to compile with Bazel 7, so maybe the USE_BAZEL_VERSION might not be needed.

Since I can't test the devcontainer stuff I can't test that myself. Passing on to @corco (who can have a look next week when back)

@kbrunham-intel : can you add a README in the directory briefly explaining what it is for ?

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.

4 participants