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 dockerFile support #349

Merged
merged 11 commits into from
Dec 18, 2024
Merged

Add dockerFile support #349

merged 11 commits into from
Dec 18, 2024

Conversation

AFOliveira
Copy link
Collaborator

@AFOliveira AFOliveira commented Dec 9, 2024

As discussed on last meeting that dockerfile could ease user interaction with the UDB, I came up with this dockerfile which runs with the following:

sudo docker build -t udb -f .devcontainer/Dockerfile .
sudo docker run -it -v $(pwd):/workspace -w /workspace udb bash

I could add an environment variable to ensure easier interaction with ./do since I have not touched that part.

Signed-off-by: Afonso Oliveira <[email protected]>
@kbroch-rivosinc
Copy link
Collaborator

This worked well for me on mac. Only thing I had to do first was:

❯ docker pull ubuntu:24.04
24.04: Pulling from library/ubuntu
Digest: sha256:80dd3c3b9c6cecb9f1667e9290b3bc61b78c2678c02cbdae5f0fea92cc6734ab
Status: Image is up to date for ubuntu:24.04
docker.io/library/ubuntu:24.04

What's next:
    View a summary of image vulnerabilities and recommendations → docker scout quickview ubuntu:24.04

Because the sudo docker build couldn't access the credentials.

After that I did the sudo docker run and was able to run the ./do commands including ./do smoke

kbroch-rivosinc
kbroch-rivosinc previously approved these changes Dec 9, 2024
@dhower-qc
Copy link
Collaborator

Excellent!

  • Why did you merge the apt commands into a single line? I though best practice was to keep them separate so that Docker can layer the image better.
  • Let's get this published to DockerHub so most people won't have to build. The singularity container is here: https://hub.docker.com/r/riscvintl/spec-generator. @rpsene, can you help us here? We'll need a place to put a x86 Docker image and an AArch64 Docker image.
  • Let's get this working with the scripts under bin/. I'm thinking we could store a selector (singularity/docker) on first run, and the use it to pick which one to use from then on. The changes should be isolated to bin/setup. @AFOliveira, is that something you can take on? We can set up a quick call to talk it through if you want.

@rpsene
Copy link
Contributor

rpsene commented Dec 10, 2024

@dhower-qc I will contribute a GH Action to periodically build and push the image to Docker Hub.

@AFOliveira
Copy link
Collaborator Author

AFOliveira commented Dec 10, 2024

The answer to this

Why did you merge the apt commands into a single line?

is my ignorance on the second part :) , but I'll modify it back.

best practice was to keep them separate so that Docker can layer the image better

Let's get this working with the scripts under bin/. I'm thinking we could store a selector (singularity/docker) on first run, and the use it to pick which one to use from then on. The changes should be isolated to bin/setup. @AFOliveira, is that something you can take on? We can set up a quick call to talk it through if you want

I can surely do it. Actually, I've kind of already done this, the way I implemented it was by setting an environment variable and just

export docker=1

and then messed around a bit with the scripts under /bin. I'll push a new commit with what I've done so far, so you can review it and maybe then we can schedule a call and discuss how to do it?

@rpsene
Copy link
Contributor

rpsene commented Dec 10, 2024

@dhower-qc @AFOliveira #351

Screenshot 2024-12-10 at 14 54 02

docker pull riscvintl/udb:latest

@AFOliveira
Copy link
Collaborator Author

FYI: I messed up the last commits, I'm fixing it.

@AFOliveira
Copy link
Collaborator Author

Actually, this is working as is. Steps to run on wsl2 are simply

export DOCKER=1
sudo -E ./do --tasks

@dhower-qc anything you would like to see differently here? We can set up a call if you prefer!

@AFOliveira AFOliveira marked this pull request as ready for review December 11, 2024 17:09
@AFOliveira
Copy link
Collaborator Author

@dhower-qc can you please write how you want this docker or specify to be done? I understood it was though a config, but what would that flow look like?

@rpsene
Copy link
Contributor

rpsene commented Dec 11, 2024

@AFOliveira take a look here: #351

I will update that PR this week.

@AFOliveira
Copy link
Collaborator Author

@AFOliveira take a look here: #351

I will update that PR this week.

Thanks (or obrigado :) ) for the pointer, Rafael. I'll be paying attention to your PR.

Copy link
Collaborator

@dhower-qc dhower-qc left a comment

Choose a reason for hiding this comment

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

@AFOliveira, I added a prompt if DOCKER/SINGULARITY env variables aren't present, and updated the regression accordingly. I also changed the Docker run command to map the UDB ROOT directly instead of $pwd (which all the scripts are assuming). Let me know if you see any issues. Otherwise, this is good to go.

@AFOliveira
Copy link
Collaborator Author

Sorry for not understanding what you were asking in first place and thanks for the help on this :).

Copy link
Collaborator Author

@AFOliveira AFOliveira left a comment

Choose a reason for hiding this comment

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

I just tested and all worked as supposed!

@AFOliveira AFOliveira merged commit 55e2cf1 into main Dec 18, 2024
10 checks passed
@AFOliveira AFOliveira deleted the AFOliveira/docker branch December 18, 2024 15:55
@AFOliveira
Copy link
Collaborator Author

@dhower-qc github pages deployment failed, did I mess this up?

@dhower-qc
Copy link
Collaborator

Ah, thanks for noticing. I forgot to update pages.yaml to pick a container, so it's just stuck at the prompt asking docker/singularity. I'll push a fix.

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