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

Update dockerfiles #771

Merged
merged 41 commits into from
Feb 21, 2021
Merged

Update dockerfiles #771

merged 41 commits into from
Feb 21, 2021

Conversation

schwarz-em
Copy link
Contributor

@schwarz-em schwarz-em commented Jan 22, 2021

Type of change: new feature + other enhancement

Impact: other

Release Notes
This adds a dockerfile with a complete chipyard set-up and its corresponding entrypoint script. The updates to the CI dockerfile use a portion of the build stages from the main dockerfile.

@abejgonzalez
Copy link
Contributor

Looks like this is pretty close to running all of the CI tests. I think we just need to add python2 to the requirements scripts for tracegen tests to run.

@abejgonzalez
Copy link
Contributor

Some other thoughts:

  • Can you also move the .circleci/images/README.md to the new dockerfile folder and write a bit about the work done?
  • It would be nice to have the CI automatically build the Dockerfile and push it up to Dockerhub (we can move that to a separate PR but it would be a nice QoL change).

@abejgonzalez abejgonzalez mentioned this pull request Feb 9, 2021
@abejgonzalez
Copy link
Contributor

abejgonzalez commented Feb 15, 2021

Looks like some tests are still failing. I would try to increase the timeout even more.

@abejgonzalez
Copy link
Contributor

Note: We will release the 1st version of this Docker image (with all deps, toolchains, etc) with 1.5.0 to Dockerhub.

Copy link
Contributor

@abejgonzalez abejgonzalez left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -18,6 +18,7 @@ sudo yum install -y centos-release-scl
sudo yum install -y devtoolset-8-make
# install DTC
sudo yum install -y dtc
sudo yum install -y python
Copy link
Contributor

Choose a reason for hiding this comment

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

A few lines above there is a python36, python36-pip and python36-devel installation. Is this python installation for an earlier or later version?

Copy link
Contributor Author

@schwarz-em schwarz-em Feb 18, 2021

Choose a reason for hiding this comment

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

It would ideally be for a earlier version. I added this because there was a bug I ran into with ubuntu-req.sh where one of the tests was looking for python2 instead of python3. I don't know a way to test on centos, so I'm not entirely sure if the bug would still occur there, so I added it just in case.

Copy link
Contributor

Choose a reason for hiding this comment

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

which test was looking for python2?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was both of the tracegen tests.

Copy link
Contributor

@alonamid alonamid left a comment

Choose a reason for hiding this comment

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

LGTM overall.
I had one question about the Python requirement, and I'm also wondering if it might be worth mentioning in the docs section that this docker image only sets up baseline Chipyard (not including FireMarshal, FireSim and Hammer initializations)

Copy link
Contributor

@tymcauley tymcauley left a comment

Choose a reason for hiding this comment

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

Looks great! Mostly I have some nitpicks from the apt-get section of this page, but those could be fixed in a subsequent PR.


# Install dependencies for ubuntu-req.sh
RUN apt-get update && \
apt-get upgrade -y && \
Copy link
Contributor

Choose a reason for hiding this comment

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

(sorry for chiming in so late)

Usually, you don't actually want to run apt-get upgrade on a Docker container. From this page:

Avoid RUN apt-get upgrade and dist-upgrade, as many of the “essential” packages from the parent images cannot upgrade inside an unprivileged container. If a package contained in the parent image is out-of-date, contact its maintainers. If you know there is a particular package, foo, that needs to be updated, use apt-get install -y foo to update automatically.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the tip. I was able to remove apt-get upgrade without any problems.

# Install dependencies for ubuntu-req.sh
RUN apt-get update && \
apt-get upgrade -y && \
apt-get install -y \
Copy link
Contributor

Choose a reason for hiding this comment

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

If you add the --no-install-recommends flag to apt-get install and conclude the command with rm -rf /var/lib/apt/lists/*, then you can help to minimize the finished container size.

RUN git clone https://github.com/ucb-bar/chipyard.git && \
cd chipyard && \
git checkout $CHIPYARD_HASH && \
./scripts/ubuntu-req.sh 1>/dev/null
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this runs apt-get ... internally, you might want to add rm -rf /var/lib/apt/lists/* at the conclusion. In general though, this page mentions that running apt-get update from two separate steps (since you run it from within this script, and also from the very first RUN command) can cause weird issues, since build caching can mean that different apt install commands are working with different apt source lists.

dockerfiles/Dockerfile Show resolved Hide resolved
dockerfiles/Dockerfile Show resolved Hide resolved
@abejgonzalez abejgonzalez self-requested a review February 19, 2021 09:38
@abejgonzalez
Copy link
Contributor

Merging (followup PR's can clean it up some more if need be... but this is currently much cleaner than the current approach).

@abejgonzalez abejgonzalez merged commit a86f801 into dev Feb 21, 2021
@schwarz-em schwarz-em deleted the update-dockerfiles branch February 21, 2021 01:05
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.

6 participants