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

Installation instructions fix: Make runt.toml expect runt version 0.4.1 instead of 0.4.0. #1698

Closed
wants to merge 7 commits into from

Conversation

nathanielnrn
Copy link
Contributor

Fixes issue when following 'Getting Started' requiring a manual version change in runt.toml. Could also be addressed in runt itself, but this is a bandaid fix for now.

@rachitnigam
Copy link
Contributor

The tests seem to be failing with this change. Maybe we need to update the GitHub workflow files to install a newer version of runt?

@nathanielnrn
Copy link
Contributor Author

Strange. The docker file installs runt using
cargo install runt --version $(grep ^ver runt.toml | awk '{print $3}' | tr -d '"')
Which is 0.4.1 with this change.

Would a new image need to be pushed to ghcr? I remember in the past that was done manually by you @rachitnigam.

@rachitnigam rachitnigam mentioned this pull request Aug 31, 2023
@rachitnigam
Copy link
Contributor

A new version of the codebase has been released which means that the docker container calyx:0.6.0 should be up soon. After its up, we need to bump the github workflow files to use it instead of calyx:0.3.0 and that should work...

@nathanielnrn
Copy link
Contributor Author

To get polybench working will need to merge this calyx-evaluation PR

Also note that docker image 0.6.0 does not have git intialized in /home/calyx.

For compiler tests, I'm getting cocotb runt errors. Which also seem to be happening on master? Even on 0.4.0. Not sure if those are expected to fail due to some other work?

@rachitnigam
Copy link
Contributor

The tests on master are passing. This might be an annoying issue to fix because of #1714. Not sure what is causing the problem but one option might be using the $HOME variable instead of hardcoding /home/calyx but can't be sure it'll work till we figure out what is causing the problem.

@nathanielnrn
Copy link
Contributor Author

nathanielnrn commented Sep 4, 2023

Failing compiler tests seem to be coming from stale
tests/frontend/systolic/array-2.expect and tests/frontend/systolic/array-3.expect
that are present in the docker container but not in the calyx repo. In 0.3.0 these are tracked and therefore removed when switching to latest commit. In 0.6.0 these are not tracked and so remain in the container.

Edit: Removing those stale files within the workflow manually worked, but not sure if it is ideal to have those commands left in the workflow explicitly, as opposed to fixing up the docker image.

@sampsyo
Copy link
Contributor

sampsyo commented Sep 8, 2023

Failing compiler tests seem to be coming from stale
tests/frontend/systolic/array-2.expect and tests/frontend/systolic/array-3.expect
that are present in the docker container but not in the calyx repo. In 0.3.0 these are tracked and therefore removed when switching to latest commit. In 0.6.0 these are not tracked and so remain in the container.

What a mess! Do we know why these files exist in the 0.6.0 container, despite not being in the repository? If some command is putting them there when the image is being built, it seems like we should probably just stop that from happening…

I also mentioned in #1714 (comment) that it seems to me like lots of this headache arises from our current CI approach, which ends up with two different versions of Calyx co-existing and potentially confusing each other. Sticking to just one (either just from the container, or just from git) could help make this stuff less surprising.

@rachitnigam
Copy link
Contributor

Let's try this again after #1824

@rachitnigam rachitnigam deleted the bump-runt-version branch February 6, 2024 06:33
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