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

treewide: Improve docker container build, CI caching and python dependencies #192

Merged
merged 130 commits into from
Sep 27, 2024

Conversation

fischeti
Copy link
Contributor

@fischeti fischeti commented Aug 30, 2024

Docker container

  • Use the official Verilator container as final base container, which has verilator prebuilt
  • Improve docker build stages for better caching
  • Clean up apt installs and getting rid of apt-requirements.txt
  • Bump python to 3.11
  • Bump Verilator to 5.020

CI

  • Build Docker container on every branch as part of the ci.yml workflow. Before, the docker container had to be manually changed and triggered on the working branch if the Docker container needed modification.
  • Cache docker build (~3min if cached, ~15min for clean build)
  • Cache Verilator simulation build if file dependencies (generated from Makefiles) remain unchanged (saves ~20min)
  • Disable redundant runs for pull_request if the pull request is from an internal contributor.
  • Temporarily disable Banshee in CI, as installation fails with a dependency error.

Python dependencies

  • Consolidate all dependencies into the pyproject.toml file, replaces all requirements.txt files.

Copy link
Collaborator

@colluca colluca left a comment

Choose a reason for hiding this comment

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

Thanks @fischeti! LGTM apart for a few comments which follow :)

.gitlab-ci.yml Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Show resolved Hide resolved
@fischeti fischeti marked this pull request as draft September 2, 2024 08:09
@fischeti
Copy link
Contributor Author

fischeti commented Sep 5, 2024

Undrafting this again after quite a lot of changes. The PR now touches a lot more than just consolidating python dependencies (see updated description).

@fischeti fischeti marked this pull request as ready for review September 5, 2024 16:44
Copy link
Collaborator

@colluca colluca left a comment

Choose a reason for hiding this comment

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

Thanks for the extensive efforts @fischeti!

It looks very good, just have a few questions/comments.

.github/workflows/build-docker.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/lint.yml Outdated Show resolved Hide resolved
iis-setup.sh Outdated Show resolved Hide resolved
util/container/Dockerfile Outdated Show resolved Hide resolved
util/container/Dockerfile Show resolved Hide resolved
util/container/Dockerfile Outdated Show resolved Hide resolved
util/container/Dockerfile Show resolved Hide resolved
@colluca colluca marked this pull request as draft September 6, 2024 12:20
@colluca colluca force-pushed the pyproject branch 4 times, most recently from 6cbe8dc to ab897df Compare September 7, 2024 09:58
@colluca
Copy link
Collaborator

colluca commented Sep 7, 2024

I pushed commit 4f89a6f to automate generation of the dependency list which determines the Verilator model build.

It is based on the List Make Prerequisites Action, which parses Make's database output (from make -pq), to generate the dependency tree for any top-level target (in our case bin/snitch_cluster.vsim). It then lists all leaf nodes of the tree, i.e. all file dependencies the target ultimately depends on, and hashes their contents.

The method is still not 100% correct, e.g. if FESVR_VERSION is changed it will not trigger a cache miss. A solution would be to include the Makefiles themselves in the list of dependencies. The List Make Prerequisites Action could be extended to this end, but probably also the Makefiles would have to be split up to mix the least amount of logic, in order not to trigger unnecessary builds on every Makefile edit.

Nonetheless, the method is at least as precise as the previously proposed method, so I think we can continue with this solution, and eventually refine it in the future, when needed.

@colluca colluca marked this pull request as ready for review September 27, 2024 10:00
@colluca colluca merged commit 379ae99 into main Sep 27, 2024
27 checks passed
@colluca colluca deleted the pyproject branch September 27, 2024 11:52
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