Skip to content

Improve act4-build image caching#1365

Open
nazar-pc wants to merge 2 commits intoriscv:act4from
nazar-pc:improve-act4-image-caching
Open

Improve act4-build image caching#1365
nazar-pc wants to merge 2 commits intoriscv:act4from
nazar-pc:improve-act4-image-caching

Conversation

@nazar-pc
Copy link
Copy Markdown
Contributor

This reduced the diff to a single layer when dependencies remain the same.

As a small piece of related feedback, it'd be nice if /work was the only writable directory during ELF building. Currently other directories are modified too.

Copilot AI review requested due to automatic review settings April 25, 2026 02:42
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Updates the ACT4 build Docker image to improve layer caching by separating dependency installation from the full repo copy, aiming to reduce rebuild cost when only source files change.

Changes:

  • Copy only tool/dependency manifest files into the dependency-warm stage to improve Docker layer cache hit rate.
  • Adjust dependency warm commands (bundle install, uv sync) and only copy the resulting /act4/.venv into the final image.
  • Move the full-repo COPY to the end of the Dockerfile (with --chmod=777) so typical source changes affect only the last layer.

Comment thread Dockerfile Outdated
Comment thread Dockerfile
Comment thread Dockerfile
Comment thread Dockerfile
@nazar-pc nazar-pc marked this pull request as draft April 25, 2026 03:00
@nazar-pc nazar-pc force-pushed the improve-act4-image-caching branch from bc9fb9b to 3acfab5 Compare April 25, 2026 03:13
@nazar-pc nazar-pc marked this pull request as ready for review April 25, 2026 03:13
Copilot AI review requested due to automatic review settings April 25, 2026 03:13
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.

Comment thread Dockerfile Outdated
Comment thread Dockerfile Outdated
@nazar-pc
Copy link
Copy Markdown
Contributor Author

#1362 caused regression for container image since the user running inside container apparently can't change permissions of files despite it it is able to read/write/delete the files, but not change permissions because of owner mismatch:

Details
ubuntu@7ba16c926d80:/act4$ CONFIG_FILES=config/cores/cve2/cv32e20/test_config.yaml make
Generating covergroups... ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━   0% -:--:--
╭─────────────────────────────────────────────────────────────────────────────────────────────── Traceback (most recent call last) ────────────────────────────────────────────────────────────────────────────────────────────────╮
│ /act4/generators/coverage/src/covergroupgen/cli.py:44 in run                                                                                                                                                                     │
│                                                                                                                                                                                                                                  │
│   41 │   ] = "",                                                                                                                                                                                                                 │
│   42 ) -> None:                                                                                                                                                                                                                  │
│   43 │   """Generate functional covergroups for RISC-V instructions from CSV testplans."""                                                                                                                                       │
│ ❱ 44 │   generate_covergroups(testplan_dir, output_dir, extensions, exclude)                                                                                                                                                     │
│   45                                                                                                                                                                                                                             │
│   46                                                                                                                                                                                                                             │
│   47 def main() -> None:                                                                                                                                                                                                         │
│                                                                                                                                                                                                                                  │
│ /act4/generators/coverage/src/covergroupgen/generate.py:592 in generate_covergroups                                                                                                                                              │
│                                                                                                                                                                                                                                  │
│   589 │   │   test_plans = all_test_plans                                                                                                                                                                                        │
│   590 │                                                                                                                                                                                                                          │
│   591 │   templates = read_covergroup_templates()                                                                                                                                                                                │
│ ❱ 592 │   write_covergroups(test_plans, templates, output_dir)                                                                                                                                                                   │
│   593 │   write_coverage_headers(all_test_plans, output_dir, templates)                                                                                                                                                          │
│   594 │   write_instruction_sample_file(all_test_plans, templates, output_dir)                                                                                                                                                   │
│   595                                                                                                                                                                                                                            │
│                                                                                                                                                                                                                                  │
│ /act4/generators/coverage/src/covergroupgen/generate.py:486 in write_covergroups                                                                                                                                                 │
│                                                                                                                                                                                                                                  │
│   483 │   │   lines.append(customize_template(templates, sample_end, arch))                                                                                                                                                      │
│   484 │   │                                                                                                                                                                                                                      │
│   485 │   │   # Write both files                                                                                                                                                                                                 │
│ ❱ 486 │   │   _write_readonly(unpriv_dir / f"{arch}_coverage.svh", "".join(lines))                                                                                                                                               │
│   487 │   │   _write_readonly(unpriv_dir / f"{arch}_coverage_init.svh", "".join(init_lines))                                                                                                                                     │
│   488                                                                                                                                                                                                                            │
│   489                                                                                                                                                                                                                            │
│                                                                                                                                                                                                                                  │
│ /act4/generators/coverage/src/covergroupgen/generate.py:54 in _write_readonly                                                                                                                                                    │
│                                                                                                                                                                                                                                  │
│    51 def _write_readonly(path: Path, content: str) -> None:                                                                                                                                                                     │
│    52 │   """Write content to a generated file and mark it read-only to deter manual edits."""                                                                                                                                   │
│    53 │   if path.exists():                                                                                                                                                                                                      │
│ ❱  54 │   │   path.chmod(_WRITABLE_MODE)                                                                                                                                                                                         │
│    55 │   path.write_text(content)                                                                                                                                                                                               │
│    56 │   path.chmod(_READONLY_MODE)                                                                                                                                                                                             │
│    57                                                                                                                                                                                                                            │
│                                                                                                                                                                                                                                  │
│ /home/shared/.local/share/uv/python/cpython-3.14.4-linux-x86_64-gnu/lib/python3.14/pathlib/__init__.py:1027 in chmod                                                                                                             │
│                                                                                                                                                                                                                                  │
│   1024 │   │   """                                                                                                                                                                                                               │
│   1025 │   │   Change the permissions of the path, like os.chmod().                                                                                                                                                              │
│   1026 │   │   """                                                                                                                                                                                                               │
│ ❱ 1027 │   │   os.chmod(self, mode, follow_symlinks=follow_symlinks)                                                                                                                                                             │
│   1028 │                                                                                                                                                                                                                         │
│   1029 │   def lchmod(self, mode):                                                                                                                                                                                               │
│   1030 │   │   """                                                                                                                                                                                                               │
╰──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╯
PermissionError: [Errno 1] Operation not permitted: 'coverpoints/unpriv/D_coverage.svh'
make: *** [Makefile:146: work/stamps/covergroupgen.stamp] Error 1


ubuntu@7ba16c926d80:/act4$ ls -alh coverpoints/unpriv/D_coverage.svh
-rwxrwxrwx 1 root root 189K Apr 25 02:39 coverpoints/unpriv/D_coverage.svh
ubuntu@7ba16c926d80:/act4$ chmod 644 coverpoints/unpriv/D_coverage.svh
chmod: changing permissions of 'coverpoints/unpriv/D_coverage.svh': Operation not permitted

In fact I'm not sure why it is trying to modify those files unconditionally all the time, that feels wrong. If you want to avoid file changes, you can commit them to the repo and then add to .gitignore, which will stop tracking further changes unless added explicitly with git add.

@nazar-pc nazar-pc force-pushed the improve-act4-image-caching branch from 3acfab5 to 5b285f1 Compare April 25, 2026 03:19
@nazar-pc
Copy link
Copy Markdown
Contributor Author

@jordancarlin what is your preference to dealing with ^ regression? Should I just delete the filed from the image and let it re-generate with correct permissions at the execution moment or will it be fixed on some other level?

@jordancarlin
Copy link
Copy Markdown
Collaborator

@jordancarlin what is your preference to dealing with ^ regression? Should I just delete the filed from the image and let it re-generate with correct permissions at the execution moment or will it be fixed on some other level?

I'm hoping #1393 should fix that. Can you give it a try?

In terms of the overall Docker image, I am thinking about doing a more significant overhaul that splits things into an act-build container that does not contain the repo and is updated only when the Dockerfile or dependencies change and a separate act4 image that includes the repo and is only published whenever there is a new release. That should help to avoid the package store bloat. Thoughts?

@nazar-pc
Copy link
Copy Markdown
Contributor Author

With this PR it mostly means moving the last line and .venv copying into a separate stage and then specifying the stage to build as target.

Though I'm more generally skeptical on usefulness of building :latest in the first place and vote for not building anything on act4 branch by default.

@nazar-pc
Copy link
Copy Markdown
Contributor Author

Let me know if you still prefer to have two images and I'll include corresponding changes in this PR.

Copilot AI review requested due to automatic review settings April 29, 2026 01:24
@nazar-pc nazar-pc force-pushed the improve-act4-image-caching branch from 5b285f1 to a02ba40 Compare April 29, 2026 01:24
@nazar-pc
Copy link
Copy Markdown
Contributor Author

Just rebased on act4, no other changes. When combined with #1393 it works as expected.

@jordancarlin
Copy link
Copy Markdown
Collaborator

There are still lots of rapid enhancements happening on the latest act4 branch, so I think it is worthwhile to make it easy for people to use the latest version. I agree that constantly rebuilding the image is unnecessary though, which is why I thought the two image approach might make more sense.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

@nazar-pc
Copy link
Copy Markdown
Contributor Author

Let me try the split in my branch then, will probably take an hour or so to run a CI build to test it on my end and I'll push it here

@nazar-pc
Copy link
Copy Markdown
Contributor Author

I think the latest revision should be doing what you suggested

@nazar-pc
Copy link
Copy Markdown
Contributor Author

nazar-pc commented May 4, 2026

@jordancarlin do you have any other feedback here?

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.

3 participants