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

Docker support #643

Open
wants to merge 1 commit into
base: sasha_ubuntu
Choose a base branch
from
Open

Docker support #643

wants to merge 1 commit into from

Conversation

save-buffer
Copy link
Contributor

Adds a Makefile rule that in tandem with the other ubuntu support PRs will be able to run regression tests inside of Docker.

@@ -38,7 +38,7 @@ endif

# To switch machines, simply switch the path of BSG_MACHINE_PATH to
# another directory with a Makefile.machine.include file.
BSG_MACHINE_PATH ?= $(BSG_F1_DIR)/machines/baseline_v0_32_16
BSG_MACHINE_PATH ?= $(BSG_F1_DIR)/machines/4x4_fast_n_fake
Copy link
Collaborator

Choose a reason for hiding this comment

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

Revert

@@ -98,8 +98,12 @@ endif # Matches: ifdef _BSG_MANYCORE_DIR
# Undefine the temporary variable to prevent its use
undefine _BSG_MANYCORE_DIR

# If we're in the docker container, verilator is installed in /usr/bin/verilator
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this because Verilator is installed through a package manager? It is best to use the one provided/compiled by bladerunner for parity.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, when building the Docker image it builds Verilator from source and installs it there.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we have to install it there? Can we just leave it?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I would rather be explicit that we're using the one in bladerunner, than install it in /usr/bin and be implicit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well the point is to not have the user build Verilator themselves - they just download the image and it'll be preinstalled. We could just not install it and keep the binary in /verilator in the image, so we'd still be explicit.

-v $(realpath $(shell pwd))/../:/bsg_bladerunner \
--env BSG_PLATFORM=dpi-verilator \
--env BSG_DOCKER=1 \
--env RISCV_BIN_DIR=/usr/bin \
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same for RISC-V Tools. I would rather use BSG_MANYCORE as the path, than use /usr/bin

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as with Verilator, BSG_MANYCORE lives in the user's filesystem, so this would be a bit difficult. We could again keep the RISCV_BIN_DIR in something like /riscv_install or similar.

@drichmond
Copy link
Collaborator

I like the idea of this.

But my fear is that the tool installation paths imposed by docker will introduce two different types of build flows, and conflicts between them.

What we've learned is valuable though. The fixes for Ubuntu alone are worth it.

@save-buffer
Copy link
Contributor Author

Well it seems like all of the build flow is made in a location-agnostic way right? Every invocation to a tool is wrapped in a variable to its path, such as with verilator we have VERILATOR=... and then later to call verilator we do $(VERILATOR). I think if for every one of these variables we just used ?=, we could have the docker script just override the location in a transparent way.

@save-buffer
Copy link
Contributor Author

Actually I think there's a hack we could do - keep things installed in the root directory in the container but then symlink them in on entry into docker. Do you think that would be better?

@drichmond
Copy link
Collaborator

I like what you said about transparency. If there's a small number of ?= changes you need to make to support Docker, then that sounds good to me. I would rather work with those changes because it provides support for Docker, but doesn't make it a supported/official part of the build flow.

Why is the docker-regression target necessary?

@save-buffer
Copy link
Contributor Author

The docker-regression target is responsible for starting docker, setting up the environment, and then running the regression target.

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